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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 10 inline comments as done.
ymandel added inline comments.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:97
+
+} // namespace
+
----------------
sgatev wrote:
> Why not put the definitions below also in the anonymous namespace?
LLVM style is to limit use of anonymous namespaces to class declarations (which have no parallel to `static`).


================
Comment at: clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:99
+
+static constexpr char kVar[] = "var";
+static constexpr char kInit[] = "init";
----------------
xazax.hun wrote:
> Do we need an actual array? I think this would copy `"var"` into a global array. Alternatively, using `static constexpr char* kVar= "var"`, we would just refer to the string in the read only memory. (Or maybe both would behave the same because compiler optimizations?)
Good question. I prefer this style because the array form carries the bounds information with it, unlike the raw pointer form: https://godbolt.org/z/jEP9Wasrh  But, I don't know of any meaningful performance implications. FWIW, this is the recommended form for our internal codebase.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:119
+
+static llvm::Optional<int64_t> transferInternal(const BoundNodes &Nodes,
+                                                const ASTContext &Context) {
----------------
sgatev wrote:
> What do you think about inlining this in `ConstantPropagationAnalysis::transfer`? We don't expect to call it from elsewhere, right?
inlined the matcher too


================
Comment at: clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:180
+
+MATCHER_P(HasConstantVal, v, "") {
+  return arg.Data.hasValue() && arg.Data->Value == v;
----------------
sgatev wrote:
> Maybe call this `HasConstVal` if you'd like to keep this short?
I'm fine with the current length. And, a little worried that "const" has its own meaning, which is separate from this one.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:217
+TEST_F(ConstantPropagationTest, JustInit) {
+  std::string code = R"(
+    void fun() {
----------------
sgatev wrote:
> Capitalize - `Code`? Or maybe inline this in the function call? Same below.
capitalized, but prefer to keep it separate for readability.  Also, generally good practice since some compliers don't allow these kinds of strings inline in macro calls.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp:245
+
+TEST_F(ConstantPropagationTest, Assignment) {
+  std::string code = R"(
----------------
sgatev wrote:
> 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?
Actually, the eval engine is more powerful than you'd guess -- 1 + 2 evaluates statically. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115740