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

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

thopre added inline comments.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:68-76
+    if (AllowUpperHex) {
+      StringRef UpperHexOnlyDigits = "ABCDEF";
+      ASSERT_TRUE(WildcardRegex.match(UpperHexOnlyDigits, &Matches));
+      EXPECT_EQ(Matches[0], UpperHexOnlyDigits);
+    } else {
+      StringRef LowerHexOnlyDigits = "abcdef";
+      EXPECT_TRUE(WildcardRegex.match(LowerHexOnlyDigits, &Matches));
----------------
jhenderson wrote:
> How about:
> 
> ```
> StringRef HexDigits = AllowUpperHex ? "ABCDEF" : "abcdef";
> EXPECT_TRUE(WildcardRegex.match(HexDigits, &Matches));
> EXPECT_EQ(Matches[0], HexDigits);
> ```
> 
> Remind me why we can't check the opposite cases are rejected here? That needs commenting at the very least.
> 
> Also, why is the check at line 70 `ASSERT_TRUE`, rather than `EXPECT_TRUE`?
It's an ASSERT because if the match fails it does not make sense checking Matches[0]. I've added negative testing, it's only for valueFromStringRepr that it is not possible (I've added a comment for that there).


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:682
+  Tester.parsePattern("[[#%u,NUMVAR_UNSIGNED:]]");
+  expectNotFoundError(Tester.match("C").takeError());
+  EXPECT_THAT_EXPECTED(Tester.match("20"), Succeeded());
----------------
jhenderson wrote:
> See comments in another review. Are this and the similar patterns below safe, if the Expected is in a success state?
With the latest previous patch yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389