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

Wed Apr 26 02:38:13 PDT 2023

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