mboehme added inline comments.
================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:96
+ for (const auto& Prop : V.properties())
+ JOS.attributeObject(Prop.first(), [&] { dump(*Prop.second); });
+
----------------
sammccall wrote:
> mboehme wrote:
> > sammccall wrote:
> > > mboehme wrote:
> > > > IIUC, this places properties on the current HTML element as attributes,
> > > > just like the built-in attributes that we add for other purposes (e.g.
> > > > "value_id", "kind").
> > > >
> > > > - What happens if we have a property whose name conflicts with one of
> > > > the built-in attributes?
> > > > - Even if we don't have a naming conflict, I think it could be
> > > > potentially confusing to have user-defined properties appear in the
> > > > same list and in the same way as built-in attributes.
> > > >
> > > > Suggestion: Can we nest all properties inside of a "properties"
> > > > attribute?
> > > >
> > > > Edit: Having looked at the HTML template now, I see that we exclude
> > > > certain attributes there ('kind', 'value_id', 'type', 'location') when
> > > > listing properties. I still think naming conflicts are a potential
> > > > problem though. I think it would also be clearer to explicitly pick the
> > > > properties out of a `properties` attribute rather than excluding a
> > > > blocklist of attributes.
> > > Right, the data model is: a value (really, a Value/StorageLocation mashed
> > > together) is just a bag of attributes.
> > >
> > > I don't think making it more complicated is an improvement: being
> > > built-in isn't the same thing as being custom-rendered.
> > > e.g. "pointee" and "truth" want the default key-value rendering despite
> > > being built-in.
> > > Having the exclude list in the template is ugly, but either you end up
> > > encoding the rendering info twice in the template like that, or you
> > > encode it once in the template and once in the JSON generation (by what
> > > goes in the "properties" map vs the main map). I'd rather call this
> > > purely a template concern.
> > >
> > > Namespace conflict could be a problem: the current behavior is that the
> > > last value wins (per JS rules).
> > > IMO the simplest fix is to prepend "p:" and "f:" to properties/struct
> > > fields. These would be shown - otherwise the user can't distinguish
> > > between a property & field with the same name.
> > >
> > > I had this in the prototype, but dropped them because they seemed a bit
> > > ugly and conflicts unlikely in practice. WDYT?
> > > Namespace conflict could be a problem: the current behavior is that the
> > > last value wins (per JS rules).
> > > IMO the simplest fix is to prepend "p:" and "f:" to properties/struct
> > > fields. These would be shown - otherwise the user can't distinguish
> > > between a property & field with the same name.
> >
> > Yes, this makes sense to me. I looked at your example screenshot and wasn't
> > sure whether they were both fields or whether one of them was a property --
> > I think there's value in indicating explicitly what they are.
> >
> > > I had this in the prototype, but dropped them because they seemed a bit
> > > ugly and conflicts unlikely in practice. WDYT?
> >
> > I do think there's a fair chance of conflicts -- many of the attribute
> > names here are short and generic and look like likely field names (e.g.
> > `kind`, `type`). Even if the chance of a conflict is relatively low, a
> > conflict will be pretty confusing when it does happen -- and given that
> > we'll be using this feature when we're debugging (i.e. already confused), I
> > think this is worth avoiding.
> >
> > One more question: How do the "p:" and "f:" items sort in the output? I
> > think these should be sorted together and grouped -- e.g. builtins first,
> > then fields, then properties. (Yes, I know this is more work... but I think
> > it's worth it.)
> > One more question: How do the "p:" and "f:" items sort in the output? I
> > think these should be sorted together and grouped -- e.g. builtins first,
> > then fields, then properties. (Yes, I know this is more work... but I think
> > it's worth it.)
>
> Javascript objects are ordered these days, so they'll display in the order we
> output them here.
> So they're already grouped, I rearranged to put properties at the end.
> > One more question: How do the "p:" and "f:" items sort in the output? I
> > think these should be sorted together and grouped -- e.g. builtins first,
> > then fields, then properties. (Yes, I know this is more work... but I think
> > it's worth it.)
>
> Javascript objects are ordered these days, so they'll display in the order we
> output them here.
Got it, thanks.
> So they're already grouped, I rearranged to put properties at the end.
SG
================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:121-123
+ for (const auto &Child : cast<StructValue>(V).children())
+ JOS.attributeObject(Child.first->getNameAsString(),
+ [&] { dump(*Child.second); });
----------------
sammccall wrote:
> mboehme wrote:
> > sammccall wrote:
> > > mboehme wrote:
> > > >
> > > this is neat but capturing the structured binding `Val` is a C++20 feature
> > Are you sure? I can see nothing here that would indicate this:
> >
> > https://en.cppreference.com/w/cpp/language/structured_binding
> >
> > And Clang doesn't complain in `-std=c++17`:
> >
> > https://godbolt.org/z/jvYE3cTdq
> Hmm, what about this :-)
>
> > Structured bindings cannot be captured by lambda expressions: (until C++20)
>
> > https://godbolt.org/z/jvYE3cTdq
>
> The capture is the problem: https://godbolt.org/z/e5P43G754
Ah, thanks -- that's the bit I didn't understand.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148949/new/
https://reviews.llvm.org/D148949
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
