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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
