This is part of the implementation of the dataflow analysis framework.
See "[RFC] A dataflow analysis framework for Clang AST" on cfe-dev.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks!
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp | ||
---|---|---|
57 | nit: invert the if and return? | |
clang/lib/Analysis/FlowSensitive/Transfer.cpp | ||
144 | nit: maybe comment to explain why we use this setting? | |
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp | ||
834 | maybe dyn_cast with ASSERT? (or somesuch) | |
919 | Why EXPECT here but ASSERT (as last line) in previous tests? | |
1040 | dyn_cast and ASSERT (or somesuch)? I'd think that whether or not we have a registered this in the environment is something we want to test? | |
1059 | why test a class distinct from a struct? Is there a meaningful difference between the two with regard to our model? if so, might it be worth instead testing explicit public vs private? |
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h | ||
---|---|---|
120 | It feels a bit wrong to have a separate ThisPointeeLoc here. But as far as I understand, this is an artifact of the AST representation we have. I think a superior AST representation would have an implicit ParmVarDecl to represent this, so we would not need to do any special handling at all, the general code path would do the right thing. I think the C++ language, with the deducing this started its approach in this direction officially. I really hope someone will have the time to fix all the mess and get rid of all the unnecessary corner cases in the foreseeable future :) | |
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp | ||
52 | There might be an optimization opportunity for initValueInStorageLocation. If we initialize a class or struct, we will end up create locations for all of the fields recursively (unless self-referential) right? Actually, in many of the codebases we cannot even access many of those fields because they might be private/protected etc. Unfortunately, it might not be easy to query whether a field is accessible from this method, but I wonder if it is worth a TODO somewhere. | |
clang/lib/Analysis/FlowSensitive/Transfer.cpp | ||
130 | Nit: I got confused for a second what will happen in a loop. I wonder if createStorageLocation is better renamed to createOrGetStorageLocation to express the fact it will not always create a new location. But I don't have strong feelings about this, also feel free to defer to a later PR. | |
140–141 | I wonder if we also want to create a non-null pointer value for these in the future so we can evaluate certain if statements. | |
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp | ||
813 | I wonder if we can make the tests a bit more concise by merging some of them. E.g. we could have a single test with both a pointer, a reference, and a value param. Although I understand that some people like to keep most tests minimal, so feel free to ignore this. | |
1062 | I'd love to see a test with multiple fields and a nested struct. |
Thank you both for the reviews!
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp | ||
---|---|---|
52 | Right. I added a FIXME in initValueInStorageLocationUnlessSelfReferential. | |
57 | I prefer to keep the block self-contained as we might end up adding code after the outer if statement. | |
clang/lib/Analysis/FlowSensitive/Transfer.cpp | ||
130 | Ack. I added a FIXME. | |
140–141 | I added a FIXME. | |
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp | ||
813 | I'll keep the tests separate for now, but I'll think about how to make them more concise. | |
919 | My bad. I updated all tests to use ASSERT only where necessary. | |
1059 | For completeness. Changing the isStructureOrClassType predicate in initValueInStorageLocationUnlessSelfReferential to isStructureType or isClassType should make one of these tests fail. I'll address public and private members in a follow up. | |
1062 | I added a nested struct and an extra member. |
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp | ||
---|---|---|
1034 | A chained access would be nice, like Qux.Baz. |
It feels a bit wrong to have a separate ThisPointeeLoc here. But as far as I understand, this is an artifact of the AST representation we have. I think a superior AST representation would have an implicit ParmVarDecl to represent this, so we would not need to do any special handling at all, the general code path would do the right thing. I think the C++ language, with the deducing this started its approach in this direction officially. I really hope someone will have the time to fix all the mess and get rid of all the unnecessary corner cases in the foreseeable future :)