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


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



================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:122
+      for (const auto &Child : cast<StructValue>(V).children())
+        JOS.attributeObject(Child.first->getNameAsString(),
+                            [&] { dump(*Child.second); });
----------------
Same rationale as for properties above: can we nest the fields inside of a `fields` attribute?

I think it would also be useful to distinguish between properties and fields in the HTML output; probably not with another nesting level (yikes!), but just rendered differently. This could easily be done if they were separated into `properties` and `fields` attributes.


================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:149
+    case Value::Kind::Biconditional: {
+      auto &VV = cast<ImplicationValue>(V);
+      JOS.attributeObject("lhs", [&] { dump(VV.getLeftSubValue()); });
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp:179
   EXPECT_THAT(Logs[0], HasSubstr("LocToVal")) << "has built-in lattice dump";
+  EXPECT_THAT(Logs[0], HasSubstr("\"type\": \"int\"")) << "has value dump";
 }
----------------
Can you add some additional tests for the following?

  - Pointers
  - References
  - Properties
  - Struct fields

Ideally, other value kinds too (see copy-paste error above for `BiconditionalValue`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148949/new/

https://reviews.llvm.org/D148949