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

Wed Jul 17 02:10:35 PDT 2019

jhenderson added a comment.

FTR, I haven't done a rigorous check of all the different new code paths to ensure you've got testing for them all, I'm afraid - the change is too large to sensibly do that, although I accept that it probably needs to be this large.

================
Comment at: llvm/lib/Support/FileCheck.cpp:40
+    // call this method if the expression associated to this variable can be
+    // evaluated.
+    assert(EvaluatedValue &&
----------------
probinson wrote:
> You should check the Expected return value with something other than an assert.  It is not sufficient to claim that the caller should call this only when the Expected result must be success. The contract for using Expected is that you will check the result. You could do something like
> ```
> if (!EvaluatedValue || *EvaluatedValue != NewValue)
>   llvm_unreachable("New value does not match expression for this variable");
> ```
> 
I think `cantFail` is intended for the kind of situation we're in here.

================
Comment at: llvm/lib/Support/FileCheck.cpp:42
+    assert(EvaluatedValue &&
+           "Cannot check new value is equal to expression for this variable");
+    assert(*EvaluatedValue == NewValue && "Value set different from evaluated");
----------------
Isn't the assertion that the `eval()` function succeeded. In that case, let's talk about that. "Failed to evaluate expression when sanity checking variable value" or something to that effect.

================
Comment at: llvm/lib/Support/FileCheck.cpp:1815
+  for (StringRef CmdlineDef : CmdlineDefines) {
+    StringRef DefIndex = Twine(++I).str();
+    size_t EqIdx = CmdlineDef.find('=');
----------------
Won't this create a StringRef to a temporary?

================
Comment at: llvm/lib/Support/FileCheck.cpp:1823
+    if (CmdlineDef[0] == '#') {
+      // Append a copy command-line definition adapted to use the same format
+      // as in the input file to be able to reuse
----------------
Append a copy of the command-line...

================
Comment at: llvm/lib/Support/FileCheck.cpp:1834
+    } else {
+      CmdlineDefsDiag += (Prefix1 + DefIndex + Prefix2).str();
+      CmdlineDefsIndices.push_back(
----------------
Prefix1 + DefIndex + Prefix2 is used twice, once in each part of the if. If you factor it out into a separate variable, it'll be much easier to read, I think, and you can then get rid of Prefix1 and Prefix2 as separate variables. You can even fold them into the place where DefIndex is created, to avoid additional non-Twine string concatenations:

`std::string SomeName = ("Global define #" + Twine(++I) + ": ").str();`

================
Comment at: llvm/lib/Support/FileCheck.cpp:1878
+          std::move(*ExpressionASTResult);
+      Expected<uint64_t> Value = ExpressionAST->eval();
+      if (!Value) {
----------------
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."

================
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

----------------
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.

================
Comment at: llvm/test/FileCheck/numeric-defines.txt:4

-RUN: FileCheck -D#NUMVAL=12 --check-prefix CHECKNUM --input-file %s %s
-RUN: not FileCheck -D#NUMVAL=8 --check-prefix CHECKNUM --input-file %s %s 2>&1 \
-RUN:   | FileCheck %s --strict-whitespace --check-prefix NUMERRMSG
-RUN: not FileCheck -D#NUMVAL=12 --check-prefix NUMNOT --input-file %s %s 2>&1 \
-RUN:   | FileCheck %s --strict-whitespace --check-prefix NOT-NUMERRMSG
-RUN: FileCheck -D#NUMVAL=8 --check-prefixes NUMNOT --input-file %s %s
+RUN: FileCheck -D#NUMVAL1=8 -D#NUMVAL2='NUMVAL1 + 4' -check-prefix CHECKNUM -input-file %s %s
+RUN: not FileCheck -D#NUMVAL2=8 -check-prefix CHECKNUM -input-file %s %s 2>&1 \
----------------
There should probably be a test case like the original, i.e. with no expression, just a literal numeric value.

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:91
+; CHECK-LABEL: EMPTY NUM EXPR
+; CHECK-NEXT: foo [[#]] bar
+
----------------
This is allowed? That's surprising to me, although I suppose it's a natural consequence of the rules.

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:172
+
+; Verify that when variable is set to an expression the expression is still
+; checked.
----------------
"when a variable"

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:174
+; checked.
+RUN: not FileCheck -check-prefix DEF-EXPR-FAIL -input-file %s %s
+
----------------
Double dashes here, please.

Is it worth checking the error message in this context, to show that the failure is due to the expression match rather than due to something to do with the definition?

================
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
----------------
Perhaps "Variable defined by numeric expression:" would make more sense.

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:88
+  // clears expression.
+  std::unique_ptr<FileCheckNumericVariableUse> FooVarUseUnique =
+      llvm::make_unique<FileCheckNumericVariableUse>("FOO", &FooVar);
----------------
"UseUnique" doesn't seem to line up with what this test is supposed to be doing, from what I can tell. What is "Unique" about it?

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:94
+      FileCheckASTBinop(doAdd, std::move(FooVarUseUnique), std::move(One));
+  FileCheckNumericVariable FoobarVar =
+      FileCheckNumericVariable("FOOBAR", 2, &Binop);
----------------
FoobarVar is the "undefined variable being set", right? In that case, I think a more purposeful name might be useful, e.g. FromVarExprVar or something like that. Happy with alternative names though.

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:332
+
+  // Valid use of variable defined on same line from expression.
+  EXPECT_FALSE(Tester.parsePatternExpect("[[#LINE1VAR:FOO+1]]"));
----------------
I might be being dumb here, but where does the "same line" come into the following 4 EXPECTs?

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:394
+  EXPECT_TRUE(Tester.matchExpect("FAIL"));
+  EXPECT_FALSE(Tester.matchExpect("18"));
+
----------------
How about EXPECT_FALSE/TRUE(Test.matchExpect(""));?

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:561
+                                      /*LineNumber=*/1, &Cxt, SM);
+  EXPECT_TRUE(bool(ExpressionAST));
+  ExpressionVal = (*ExpressionAST)->eval();
----------------
There are probably many other places where this is a problem already, but this should be ASSERT_TRUE, since the test cannot continue if it fails, and will crash on the next line (probably). Perhaps a separate commit to clean those up would be appropriate.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60388