martong 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) {}
----------------
gribozavr2 wrote:
> Please use CRTP (a non-virtual function here), and you'll need SFINAE to
> detect the presence of the overload of branchTransfer in
> branchTransferTypeErased.
Ok, changed it to use CRTP.
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:86
Environment &) = 0;
+ virtual void branchTransferTypeErased(bool Branch, const Stmt *,
+ TypeErasedLattice &, Environment &) =
0;
----------------
gribozavr2 wrote:
> 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...`?
Ok, added a doc comment and renamed to transferBranch.
================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:148
+ bool Assumption = true;
// The condition must be inverted for the successor that encompasses the
----------------
gribozavr2 wrote:
> `CondValue`?
Ok, changed to `ConditionValue`.
================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:253
+ auto [Cond, Assumption] =
+ TerminatorVisitor(Analysis, StmtToEnv, PredState.Env,
+ blockIndexInPredecessor(*Pred, Block),
----------------
gribozavr2 wrote:
> It would be nice to still call `branchTransfer` even when
> `BuiltinTransferOpts == false`. Please leave a TODO if you don't want to
> implement that.
Ok, I've added a TODO.
================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:241
+ ? SignLattice(R.Val.getInt().getExtValue())
+ : SignLattice::bottom();
+ } else {
----------------
dkrupp wrote:
> Isn't this SignLattice::top() instead?
>
> This is an initialization expression, which we cannot evaluate to int, but
> the variable is initialized.
I am moving the sign analysis out of this patch.
================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:586
+ HoldsSignLattice(UnorderedElementsAre(Pair(Var("a"),
Top()))))));
+}
+
----------------
gribozavr2 wrote:
> 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?
Ok. I've removed the sign analysis from this patch and added a very simple test
file with a very simple lattice to check the effect of transferBranch.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp:60-66
+MATCHER_P(Var, name,
+ (llvm::Twine(negation ? "isn't" : "is") + " a variable named `" +
+ name + "`")
+ .str()) {
+ assert(isa<VarDecl>(arg));
+ return arg->getName() == name;
+}
----------------
This is unused (a leftover). I am going remove this matcher.
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
