gribozavr2 added inline comments.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:127
+ // Default implementation is a Noop.
+ virtual void branchTransfer(bool Branch, const Stmt *S, Lattice &L,
+ Environment &Env) {}
----------------
Please use CRTP (a non-virtual function here), and you'll need SFINAE to detect
the presence of the overload of branchTransfer in branchTransferTypeErased.
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:86
Environment &) = 0;
+ virtual void branchTransferTypeErased(bool Branch, const Stmt *,
+ TypeErasedLattice &, Environment &) =
0;
----------------
Please add a doc comment. Please add a parameter name for the condition, it is
not obvious what it is.
Could you think of a more descriptive name for `Branch`? For example, if the
second parameter is `Cond`, WDYT about `CondValue`?
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:86
Environment &) = 0;
+ virtual void branchTransferTypeErased(bool Branch, const Stmt *,
+ TypeErasedLattice &, Environment &) =
0;
----------------
gribozavr2 wrote:
> Please add a doc comment. Please add a parameter name for the condition, it
> is not obvious what it is.
>
> Could you think of a more descriptive name for `Branch`? For example, if the
> second parameter is `Cond`, WDYT about `CondValue`?
>
>
WDYT about calling it `transferBranch...` instead of `branchTransfer...`?
================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:78
public:
- TerminatorVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env,
+ using RetTy = std::pair<const Expr *, bool>;
+ TerminatorVisitor(TypeErasedDataflowAnalysis &Analysis,
----------------
Please add a comment to RetTy.
================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:148
+ bool Assumption = true;
// The condition must be inverted for the successor that encompasses the
----------------
`CondValue`?
================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:253
+ auto [Cond, Assumption] =
+ TerminatorVisitor(Analysis, StmtToEnv, PredState.Env,
+ blockIndexInPredecessor(*Pred, Block),
----------------
It would be nice to still call `branchTransfer` even when `BuiltinTransferOpts
== false`. Please leave a TODO if you don't want to implement that.
================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:586
+ HoldsSignLattice(UnorderedElementsAre(Pair(Var("a"),
Top()))))));
+}
+
----------------
This file is an amazing example of how to use the framework, but could you add
a more direct unit test with a simpler lattice and analysis that exercises the
new callback?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133698/new/
https://reviews.llvm.org/D133698
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
