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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel requested changes to this revision.
ymandel added a comment.
This revision now requires changes to proceed.

Overall, looks quite good.



================
Comment at: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:87-88
 
+  /// Applies the analysis transfer function for a given edge from a
+  /// CFG block of a conditional statement.
+  /// @param Stmt The condition which is responsible for the split in the CFG.
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:78-79
 public:
-  TerminatorVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env,
+  // The return type of the visit functions.
+  using RetTy = std::pair<const Expr *, bool>;
+  TerminatorVisitor(TypeErasedDataflowAnalysis &Analysis,
----------------
please lift this out and define it as a struct. then, refer to it by name on line 76.  that will improve the readability of the code and provide a way to document explicitly the role of the two fields.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:258
+                .Visit(PredTerminatorStmt);
+        if (Cond)
+          // TODO Call transferBranchTypeErased even if BuiltinTransferOpts are
----------------
I generally prefer explicit comparison but it's particularly important here, given that `Cond` sounds like it could be a boolean.


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:259
+        if (Cond)
+          // TODO Call transferBranchTypeErased even if BuiltinTransferOpts are
+          // not set.
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp:28
+using namespace ast_matchers;
+using namespace test;
+
----------------
feels odd to mix and match namespace handling here. I think that if you want to go with `namespace clang...` that's fine but then do the same for `test`. In fact, since we're now in C++17, maybe:
```
namespace clang::dataflow::test {
namespace {
```


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp:31
+struct TestLattice {
+  enum class Branch : int { True, False };
+  llvm::Optional<Branch> TheBranch;
----------------
bool? i understand that you don't want to use bool directly, but still seems like the better base type here.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp:86-88
+          [&Match](const llvm::StringMap<DataflowAnalysisState<TestLattice>>
+                       &Results,
+                   const AnalysisOutputs &AO) { Match(Results, AO.ASTCtx); }),
----------------
Can your test below use `AnalysisOutputs` directly, and thereby avoid this conversion lambda?


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp:120
+        const TestLattice &LP = getLatticeAtAnnotation(Results, "p");
+        EXPECT_TRUE(LP.TheBranch == TestLattice::Branch::True);
+
----------------
Please be explicit about the optional, since this is an important detail of the test.   Same below.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp:123
+        const TestLattice &LQ = getLatticeAtAnnotation(Results, "q");
+        EXPECT_TRUE(LQ.TheBranch == TestLattice::Branch::False);
+      },
----------------
EXPECT_THAT...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133698/new/

https://reviews.llvm.org/D133698