[PATCH] D60385: FileCheck [5/12]: Introduce regular numeric variables

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I may have missed it, but I'm not sure I saw any unit tests for parsing a numeric expression involve an addition or subtraction?



================
Comment at: llvm/test/FileCheck/var-scope.txt:1
+; Test that variables not starting with dollar sign get undefined after a
+; CHECK-LABEL directive iff --enable-var-scope is used.
----------------
This test has the same mixed -/-- as mentioned in other test cases. Please fix!


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:23
+  EXPECT_TRUE(Value);
+  EXPECT_EQ((uint64_t)42, *Value);
+  EXPECT_TRUE(FooVar.setValue((uint64_t)43));
----------------
thopre wrote:
> jhenderson wrote:
> > What's the purpose of the cast here and elsewhere in this test?
> To avoid a warning about comparison between sined (42) and unsigned (*Value which is uint64_t) integers. Elsewhere I've suffixed the literal with U, I've now normalized and that approach (ie. 42U in this case)
Okay. I feel like it's a bug if that warning comes out and the signed number is known to be a positive literal, but that's a separate issue.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:18
+TEST_F(FileCheckTest, NumericVariable) {
+  FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 42);
+
----------------
Maybe worth adding a check of `getName` here.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:24
+  EXPECT_EQ(42U, *Value);
+  EXPECT_TRUE(FooVar.setValue(43));
+
----------------
Check getValue() here after setting, to show that the new value is returned for a defined variable.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:41
+
+TEST_F(FileCheckTest, NumExprEvalUndef) {
+  FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 42);
----------------
Undef? Maybe this test should be just called NumExpr, since it tests the entire class.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:45
+
+  // Defined variable: eval returns right value, no undef variable returned.
+  llvm::Optional<uint64_t> Value = NumExpr.eval();
----------------
undef -> undefined here and below


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:58
+  EXPECT_EQ("FOO", UndefVar);
+}
+
----------------
I might be worth showing somewhere that getUndefVarName can be called without an eval call.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:183
 
+  // Defined variable
+  VarName = "FOO";
----------------
Nit: full stop here and next comment.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:228-229
 
-  FileCheckNumExpr NumExpr = FileCheckNumExpr(42);
+  // Substitution of defined numeric variable returns the right value.
+  FileCheckNumericVariable LineVar = FileCheckNumericVariable("@LINE", 42);
+  FileCheckNumExpr NumExpr = FileCheckNumExpr(doAdd, &LineVar, 0);
----------------
I've lost track a little. Is @LINE the only allowed numeric variable currently, or can we have others now (I thought we could)? If we can, we should have a non-pseudo variable test case here (possibly in addition to a pseudo one).


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:255
 
+  // Undef var in pattern variable substitution with undefined variable returns
+  // the variable.
----------------
Explicitly say that "getUndefVarName returns ..." instead of "Undef var ..." here and below.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:275
+
+  // Undef var in valid numeric expression substitution is empty.
+  // Undef var in numeric expression substitution with undefined variable
----------------
What is this comment line referring to?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:361-363
+  // Check eval fails even if we kept a pointer to the numeric expression since
+  // clearing of local variable because of --enable-var-scope happens after
+  // numeric expressions are linked to the numeric variables they use.
----------------
variable -> variables

Based on your comment, I think this can be further improved:

"Check a numeric expression's eval fails if called after clearing of local variables, if it was created before. This is important because local variable clearing because of --enable-var-scope happens after numeric expressions are linked to the numeric variables they use."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60385