sgatev added inline comments.
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:69
+
+ friend bool operator==(ConstantPropagationLattice Element1,
+ ConstantPropagationLattice Element2) {
----------------
Should this be a const ref? Same below.
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:97
+
+} // namespace
+
----------------
Why not put the definitions below also in the anonymous namespace?
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:119
+
+static llvm::Optional<int64_t> transferInternal(const BoundNodes &Nodes,
+ const ASTContext &Context) {
----------------
What do you think about inlining this in
`ConstantPropagationAnalysis::transfer`? We don't expect to call it from
elsewhere, right?
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:160
+
+ ConstantPropagationLattice transfer(const Stmt *Stmt,
+ ConstantPropagationLattice Element,
----------------
Maybe call this `S` to disambiguate from the name of the type?
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:161
+ ConstantPropagationLattice transfer(const Stmt *Stmt,
+ ConstantPropagationLattice Element,
+ Environment &Env) {
----------------
Should this be a const ref?
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:180
+
+MATCHER_P(HasConstantVal, v, "") {
+ return arg.Data.hasValue() && arg.Data->Value == v;
----------------
Maybe call this `HasConstVal` if you'd like to keep this short?
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:197
+protected:
+ ConstantPropagationTest() = default;
+
----------------
Is this necessary?
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:217
+TEST_F(ConstantPropagationTest, JustInit) {
+ std::string code = R"(
+ void fun() {
----------------
Capitalize - `Code`? Or maybe inline this in the function call? Same below.
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:245
+
+TEST_F(ConstantPropagationTest, Assignment) {
+ std::string code = R"(
----------------
Maybe add tests for assignment from a function call (e.g. `int target =
foo();`) and assignment from an arithmetic expression (e.g. `int target = 1 +
2;`)? Both should result in `IsUnknown()` state, right?
================
Comment at:
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:342
+ if (b) {
+ target = 1;
+ } else {
----------------
Add internal checks, similar to those in `SameAssignmentInBranches`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115740/new/
https://reviews.llvm.org/D115740
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
