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
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h | ||
---|---|---|
41 | I think S would be easier to read. | |
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h | ||
244–245 | ||
253–254 | ||
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp | ||
34 | Why not use try_emplace earlier, to save of one of the lookups? | |
clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp | ||
28–31 | maybe add another one which simply tests that two calls to createAtomicBoolValue return distinct values (or even just distinct pointers?) |
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h | ||
---|---|---|
41 | Initially, I was wondering if we want to make the solver optional to potentially speed up analyses that do not need this capability. But I realized, we could just pass in a dummy no-op solver in those cases so this should be ok. | |
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h | ||
247 | Should we move this check to DataflowAnalysisContext to protect against cases where users inadvertently try to call the its methods directly instead of using the Environment? | |
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp | ||
30 | Alternatively, we could canonicalize BoolValues, e.g. always having the operand with the smaller address on the left. This could help us doing one less lookup (at the cost of a pointer comparison and sometimes a swap). |
Address reviewers' comments.
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h | ||
---|---|---|
41 | Agreed. | |
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h | ||
247 | Done, but kept the implication and bi-conditional helpers in Environment as I'm not sure if we'll need them in DataflowAnalysisContext at this point. | |
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp | ||
30 | Agreed. I updated the implementation. | |
34 | Makes sense. I updated the implementation. | |
clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp | ||
28–31 | Done. |
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp | ||
---|---|---|
44 | As far as I understand, we need to do the double lookup (find and later try_emplace) because we do not want to allocate + take ownership when we hit the cache. An alternative is to first do try_emplace with a nullptr and overwrite the value if the emplace was successful. But I guess that also can be confusing, so it is up to you which one would you prefer. |
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp | ||
---|---|---|
44 | good point. I was thinking of the value case. Sorry. |
Thanks!
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp | ||
---|---|---|
44 | Makes sense and I think it doesn't look that bad so let's go with it. |
I think S would be easier to read.