[PATCH] D60389: FileCheck [9/12]: Add support for matching formats

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre marked an inline comment as done.
thopre added inline comments.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:340
   FileCheckPatternContext Context;
-  Pattern P{Check::CheckPlain, &Context, LineNumber++};
+  Pattern P{Check::CheckPlain, &Context, LineNumber};
 
----------------
jhenderson wrote:
> This '++' change, and the corresponding changes below, seem unrelated?
Indeed, made it into a separate patch: https://reviews.llvm.org/D72913


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:461-463
+  EXPECT_TRUE(Tester.parsePatternExpect("[[@LINE+FOO]]"));
+  EXPECT_TRUE(Tester.parsePatternExpect("[[2+ at LINE]]"));
+  EXPECT_TRUE(Tester.parsePatternExpect("[[@LINE+0xC]]"));
----------------
jhenderson wrote:
> I've lost track. What's the difference between `parsePatternExpect` and `parseSubstExpect`? Why is it pattern, not subst here? Finally, what do these test cases have to do with formats?
parsePatternExpect will call parsePattern which parse the rhs of a CHECK directive. parseSubstExpect calls parseNumericSubstitutionBlock which parses what's inside a [[#]] block. The use of parsePatternExpect is because I'm testing that legacy @LINE expression only accept what the old [[@LINE]] (without #) accepted before this patch set. The restriction for legacy @LINE expression is in parseNumericSubstitutionBlock and private functions it calls but the detection of a legacy @LINE expression is in parsePattern hence the use of parsePatternExpect.

The last of the 3 tests is format related because this patch also enables hex literals (I didn't want to make a separate patch for a few line diff). The other 2 should have been done in earlier patch. I've split these 2 into a separate patch: https://reviews.llvm.org/D72912


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:760
   // expressions are linked to the numeric variables they use.
-  EXPECT_TRUE(errorToBool((*ExpressionASTPointer)->eval().takeError()));
+  EXPECT_TRUE(errorToBool((*ExpressionPointer)->getAST()->eval().takeError()));
   P = Pattern(Check::CheckPlain, &Cxt, 2);
----------------
jhenderson wrote:
> I know this isn't something directly related to your change, so it should be a later one, but you should also remove all uses of errorToBool in favour of checking the actual Error.
> 
> Sorry I didn't pick up on those in earlier reviews.
Done in https://reviews.llvm.org/D72914


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389