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:
> > 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.)
================
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:
> >
> 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
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
