[PATCH] D148949: [dataflow] HTMLLogger - show the value of the current expr

[Martin Böhme via Phabricator via cfe-commits]

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