sammccall marked an inline comment as done.
sammccall 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); });
+
----------------
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?
================
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); });
----------------
mboehme wrote:
>
this is neat but capturing the structured binding `Val` is a C++20 feature
================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:149
+ case Value::Kind::Biconditional: {
+ auto &VV = cast<ImplicationValue>(V);
+ JOS.attributeObject("lhs", [&] { dump(VV.getLeftSubValue()); });
----------------
mboehme wrote:
>
right, thanks :-(
Can't wait to get rid of this stuff :-)
================
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";
}
----------------
mboehme wrote:
> 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`).
There isn't a reasonable way to test in that level of detail without paying a
lot for it.
String assertions on the HTML are not suitable for testing nontrivial
structures in the JSON.
If we restructure to allow such assertions on the JSON:
- it would be significantly expensive/intrusive
- we would be relying on details of the analysis itself and setting them up
indirectly - quite brittle
- we would spend a lot on testing and still not look at all at what matters
(the rendering)
My experience is that not having proper tests for these kind of tools is the
least-worst option: better than brittle tests where true vs spurious errors are
unclear, and better than expensive+still incomplete tests.
The debugging tools tend to get changed infrequently, manually verified, and
sometimes an annoying regression creeps in.
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
