sammccall 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); });
+
----------------
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.
================
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); });
----------------
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
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
