[PATCH] D60388: FileCheck [8/12]: Define numeric var from expr

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1878
+          std::move(*ExpressionASTResult);
+      Expected<uint64_t> Value = ExpressionAST->eval();
+      if (!Value) {
----------------
jhenderson wrote:
> jhenderson wrote:
> > Why is the expression being evaluated here? This probably needs a comment.
> "**Now evaluate** the **expression,** whose value this variable should be set **to,** since the expression of a command-line variable definition should only use **variables** defined earlier on the command-line."
This comment hasn't been addressed.


================
Comment at: llvm/lib/Support/FileCheck.cpp:38
+    // Caller is expected to call setValue only if substitution was successful.
+    assert(NewValue == cantFail(ExpressionAST->eval(), "Failed to evaluate associated expression when sanity checking value") && "Value set different from evaluated");
+  }
----------------
clang-format?

Rephrase the second string to "Value being set to different from variable evaluation"


================
Comment at: llvm/test/FileCheck/numeric-defines-diagnostics.txt:5
 RUN: not FileCheck -D#10VALUE=10 --input-file %s %s 2>&1 \
 RUN:   | FileCheck %s --strict-whitespace --check-prefix NUMERRCLIFMT
 
----------------
jhenderson wrote:
> By the way, if you were to use --match-full-lines, you wouldn't need to use the regular expression in the last line of each of these checks, and your arrow would point closer to where it should be.
This has been marked as Done, but I don't see any changes?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:85
 
+  // Undefined variable set from numeric expression: isValueKnownAtMatchTime
+  // returns true, getValue and eval return value of expression, setValue
----------------
jhenderson wrote:
> Perhaps "Variable defined by numeric expression:" would make more sense.
I wanted the "Undefined" bit deleting. I don't think it's needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60388