[PATCH] D115740: [clang][dataflow] Add simplistic constant-propagation analysis.

[Stanislav Gatev via Phabricator via cfe-commits]

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