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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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));
----------------
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`?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:94-105
+  if (AllowHex) {
+    if (AllowUpperHex) {
+      EXPECT_EQ(*TenMatchingString, "A");
+      EXPECT_EQ(*FifteenMatchingString, "F");
+    } else {
+      EXPECT_EQ(*TenMatchingString, "a");
+      EXPECT_EQ(*FifteenMatchingString, "f");
----------------
I'd recommend the following, to reduce duplication of checks.
```
StringRef Ten;
StringRef Fifteen;
if (AllowHex) {
  if (AllowUpperHex) {
    Ten = "A";
    Fifteen = "F";
  } else {
    Ten = "a";
    Fifteen = "f";
  }
} else {
  Ten = "10";
  Fifteen = "15;
}
EXPECT_EQ(*TenMatchingString, Ten);
EXPECT_EQ(*FifteenMatchingString, Fifteen);
```


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:156
+
+TEST_F(FileCheckTest, InvalidExplicitFormatsProperties) {
+  ExpressionFormat NoFormat(ExpressionFormat::Kind::NoFormat);
----------------
Formats -> Format


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:163
+  EXPECT_FALSE(bool(NoFormat));
+  ExpressionFormat ConflictFormat(ExpressionFormat::Kind::Conflict);
+  expectError<StringError>("trying to match value with invalid format",
----------------
Probably worth a blank line to separate out the Conflict and NoFormat cases. Or just factor them into separate tests.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:171
+
+TEST_F(FileCheckTest, FormatsEqualityOperators) {
+  // Check format equality operators.
----------------
Formats -> Format


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:172
+TEST_F(FileCheckTest, FormatsEqualityOperators) {
+  // Check format equality operators.
+  ExpressionFormat UnsignedFormat(ExpressionFormat::Kind::Unsigned);
----------------
This comment is unnecessary - the test code is self-documenting.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:177
+  EXPECT_FALSE(UnsignedFormat != UnsignedFormat2);
+  ExpressionFormat HexLowerFormat(ExpressionFormat::Kind::HexLower);
+  EXPECT_FALSE(UnsignedFormat == HexLowerFormat);
----------------
Put a line break above the start of each case, i.e. at the start of each (set of) declaration(s). This will show which the cases more distinctly.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:189
+
+  // Check format versus kind equality operators.
+  EXPECT_TRUE(UnsignedFormat == ExpressionFormat::Kind::Unsigned);
----------------
On further thought, consider replacing this comment with a new TEST, with the test naem documenting the test purpose.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:291
 
-  // Defined variable: eval returns right value.
+  // Defined variables: eval returns right value, implicit format is as
+  // expected.
----------------
, -> ;


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:618
+
+  // Valid expression with several variables when their implicit format do not
+  // conflict.
----------------
format -> formats


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:622
+
+  // Valid implicit format conflict in presence of explicit format.
+  EXPECT_THAT_EXPECTED(Tester.parseSubst("%X,FOO+VAR_LOWER_HEX"), Succeeded());
----------------
format -> formats


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:663
+  // Invalid legacy @LINE expression with non decimal literal.
+  EXPECT_FALSE(Tester.parsePattern("[[#@LINE+2]]"));
 }
----------------
What's the diference between this test case and the one above?


================
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());
----------------
See comments in another review. Are this and the similar patterns below safe, if the Expected is in a success state?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:914-915
   // expressions are linked to the numeric variables they use.
-  EXPECT_THAT_EXPECTED((*ExpressionASTPointer)->eval(), Failed());
+  EXPECT_THAT_ERROR((*ExpressionPointer)->getAST()->eval().takeError(),
+                    Failed());
   P = Pattern(Check::CheckPlain, &Cxt, 3);
----------------
Should this test the actual Error contents, like you do elsewhere?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:930
   EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
   EXPECT_THAT_EXPECTED(EmptyVar, Failed());
   // Clear again because parseNumericSubstitutionBlock would have created a
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389