[PATCH] D133698: [clang][dataflow] Implement transferBranch

[Gabor Marton via Phabricator via cfe-commits]

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