[PATCH] D72912: [FileCheck] Clean and improve unit tests

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:296
+  // Invalid use of variable defined on the same line. Use parsePatternExpect
+  // for the variable defined to become visible. Note that the same pattern
+  // object is used for the parsePatternExpect and parseSubstExpect since no
----------------
It's not clear to me exactly what the phrase "for the variable defined to become visible" means. What does it mean to be "visible"? Also, I suspect you mean "the defined variable" or even just "the variable".


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:308-309
+
+  // Valid use of undefined variable which creates the variable and make it
+  // visible.
+  ASSERT_FALSE(Tester.parseSubstExpect("UNDEF"));
----------------
Again, what does "visible" mean? Also "make it visible" -> "makes it visible".


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:325
 
-  // Missing offset operand.
+  // Missing rhs operand.
   EXPECT_TRUE(Tester.parseSubstExpect("@LINE+"));
----------------
Acronyms like this should generally be capitalized, i.e. "RHS".


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:329
+  // Invalid rhs operand.
+  EXPECT_TRUE(Tester.parseSubstExpect("@LINE+%VAR"));
+
----------------
This case feels like it's covering two bases, making it unclear what the actual cause of failure is. "%VAR" is not a valid name, right, plus I thought @LINE didn't support variables. Should this be "@LINE+FOO"?

Of course, looking below makes me think I'm wrong, which suggests the comment needs updating to say why the RHS is invalid.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:337-339
+  // Valid expression with 2 or more operands.
+  EXPECT_FALSE(Tester.parseSubstExpect("FOO+3"));
+  EXPECT_FALSE(Tester.parseSubstExpect("FOO+3- at LINE"));
----------------
I'm concerned that these are getting mixed up with the @LINE test cases. The first at least should probably be separate and earlier. The latter might belong somewhere around here however.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:353
   EXPECT_TRUE(Tester.parsePatternExpect("[[#42INVALID]]"));
-  EXPECT_TRUE(Tester.parsePatternExpect("[[#@FOO]]"));
-  EXPECT_TRUE(Tester.parsePatternExpect("[[#@LINE/2]]"));
----------------
It's not clear to me how this and the other deleted test cases are covered elsewhere?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:399
+
+  // Check matching the variable defined matches the correct number only.
+  Tester.initNextPattern();
----------------
the variable defined -> a defined variable


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:441
 
-  // Substitutions of defined pseudo and non-pseudo numeric variables return
-  // the right value.
-  NumericVariable LineVar("@LINE", 1);
+  // Substitutions of defined numeric variables return its value.
   NumericVariable NVar("N", 1);
----------------
return its value -> match their value


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:445
   auto NVarUse = std::make_unique<NumericVariableUse>("N", &NVar);
-  NumericSubstitution SubstitutionLine(&Context, "@LINE", std::move(LineVarUse),
-                                       12);
   NumericSubstitution SubstitutionN(&Context, "N", std::move(NVarUse), 30);
   SubstValue = SubstitutionN.getResult();
----------------
What's the 30 here?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:547
   Expected<StringRef> LocalVar = Cxt.getPatternVarValue(LocalVarStr);
-  Pattern P(Check::CheckPlain, &Cxt, 1);
+  P = Pattern(Check::CheckPlain, &Cxt, 2);
   Optional<NumericVariable *> DefinedNumericVariable;
----------------
Should this 2 and the 2 below in the parseNumericSubstitution block match? If so, consider pulling them both out into a separate variable. Same applies below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72912