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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/FileCheck/pattern-defines-diagnostics.txt:15
 
 ; Missin variable name.
 RUN: not FileCheck -D=10 --input-file %s %s 2>&1 \
----------------
Nit: Missin -> missing (sorry, didn't see this before)


================
Comment at: llvm/test/FileCheck/var-scope.txt:4-6
+RUN: FileCheck -D#LOCNUM=1 -D'#$GLOBNUM=1' -input-file %s %s
+RUN: FileCheck -D#LOCNUM=1 -D'#$GLOBNUM=1' -check-prefixes CHECK,GLOBAL -input-file %s %s
+RUN: FileCheck -D#LOCNUM=1 -D'#$GLOBNUM=1' -check-prefixes CHECK,LOCAL3 -input-file %s %s
----------------
What's the purpose of having these 3 as separate RUN lines, rather than a single one with check-prefixes=CHECK,GLOBAL,LOCAL3?


================
Comment at: llvm/test/FileCheck/verbose.txt:135
 VV-NEXT: {{C}}HECK-NOT: {{[{][{]z[}][}]yx$}}
-VV-NEXT:   {{^                 \^$}}
+VV-NEXT:   {{^                  \^$}}
 VV-NEXT: verbose.txt:[[@LINE+13]]:1: note: found here
----------------
Why has this changed?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:23
+  EXPECT_TRUE(Value);
+  EXPECT_EQ((uint64_t)42, *Value);
+  EXPECT_TRUE(FooVar.setValue((uint64_t)43));
----------------
What's the purpose of the cast here and elsewhere in this test?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:42
+TEST_F(FileCheckTest, NumExprEvalUndef) {
+  auto FooVar = new FileCheckNumericVariable("FOO", 42);
+  auto NumExpr = new FileCheckNumExpr(doAdd, FooVar, 18);
----------------
Why is this a pointer here?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:43
+  auto FooVar = new FileCheckNumericVariable("FOO", 42);
+  auto NumExpr = new FileCheckNumExpr(doAdd, FooVar, 18);
+
----------------
Why is this a pointer?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:147
   SourceMgr SM;
+  struct FileCheckRequest Req;
   FileCheckPatternContext Context;
----------------
Why `struct`?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:229
+  // Substitution of defined numeric variable returns the right value.
+  auto LineVar = new FileCheckNumericVariable("@LINE", 42);
+  FileCheckNumExpr NumExpr = FileCheckNumExpr(doAdd, LineVar, 0);
----------------
Why is this a pointer?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:269
+  // empty.
+  auto LineVar = new FileCheckNumericVariable("@LINE", 42);
+  FileCheckNumExpr NumExpr = FileCheckNumExpr(doAdd, LineVar, 0);
----------------
Why is this a pointer?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:295
 
-  // Empty variable
+  // Empty variable.
   GlobalDefines.clear();
----------------
"Empty variable name"?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:303
 
-  // Invalid variable
+  // Invalid variable.
   GlobalDefines.clear();
----------------
"Invalid variable name"?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:352
+  EXPECT_TRUE(NumExprVal);
+  EXPECT_EQ(*NumExprVal, 18U);
   EXPECT_TRUE(EmptyVar);
----------------
Do you need to be explicit about the unsigned-ness here?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:361-366
+  // Check eval fails even if we kept a pointer to the numeric expression.
+  EXPECT_FALSE(NumExpr->eval());
+  P = FileCheckPattern(Check::CheckPlain, &Cxt);
+  NumExpr =
+      P.parseNumericExpression(LocalNumVarRef, false /*IsPseudo*/, "", SM);
+  EXPECT_FALSE(NumExpr);
----------------
I'm not sure I follow the purpose of this test case addition.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:386
+  EXPECT_TRUE(NumExprVal);
+  EXPECT_EQ(*NumExprVal, 36U);
 
----------------
Is it necessary to be explicit about the unsigned-ness here?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:398
+  EXPECT_TRUE(NumExprVal);
+  EXPECT_EQ(*NumExprVal, 36U);
 }
----------------
Is it necessary to be explicit about the unsigned-ness here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60385