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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
