sgatev added inline comments.
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:41
+ ///
+ /// `Slvr` must not be null.
+ DataflowAnalysisContext(std::unique_ptr<Solver> Slvr)
----------------
xazax.hun wrote:
> ymandel wrote:
> > I think `S` would be easier to read.
> 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.
Agreed.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:247
+ BoolValue &makeAnd(BoolValue &LHS, BoolValue &RHS) {
+ return &LHS == &RHS ? LHS : DACtx->getOrCreateConjunctionValue(LHS, RHS);
+ }
----------------
xazax.hun wrote:
> 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`?
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.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:30
+
+ It = ConjunctionVals.find({&RHS, &LHS});
+ if (It != ConjunctionVals.end())
----------------
xazax.hun wrote:
> Alternatively, we could canonicalize `BoolValue`s, 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).
Agreed. I updated the implementation.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:34
+
+ auto Res = ConjunctionVals.try_emplace(
+ {&LHS, &RHS},
----------------
ymandel wrote:
> Why not use `try_emplace` earlier, to save of one of the lookups?
Makes sense. I updated the implementation.
================
Comment at:
clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp:28-31
+TEST_F(DataflowAnalysisContextTest,
+ GetOrCreateConjunctionValueReturnsSameExprOnSubsequentCalls) {
+ auto &X = Context.createAtomicBoolValue();
+ auto &Y = Context.createAtomicBoolValue();
----------------
ymandel wrote:
> maybe add another one which simply tests that two calls to
> `createAtomicBoolValue` return distinct values (or even just distinct
> pointers?)
Done.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120711/new/
https://reviews.llvm.org/D120711
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
