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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

No problem with the constness changes.



================
Comment at: llvm/lib/Support/FileCheckImpl.h:71
+
+  bool operator==(const Kind OtherValue) const { return Value == OtherValue; }
+
----------------
arichardson wrote:
> Since `Kind` is just an enum, the const is unnecessary for by-value arguments but I don't think it does any harm either.
It won't do any harm, I think, but it's inconsistent with style elsewhere, so I think it should be deleted (the const on the method is fine though of course).


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:32
+
+  // Check methods output for unsigned decimal format.
+  ExpressionFormat UnsignedFormat(ExpressionFormat::Kind::Unsigned);
----------------
Nit: this comment isn't quite right. You probably wanted "methods' output", since its the output of the methods. However, I think a clearer phrase this and the similar comments to "Check unsgined decimal format methods", or possibly even "Check unsigned decimal format properties".

In fact, now that I think about it, perhaps it would make more sense for this test to be split into a separate TEST for each different format. The test name would then document what the test is for, and you wouldn't need the comment. You might even want to consider a parameterised test (see the TEST_P function), to avoid code duplication, for the Unsigned, HexLower and HexUpper cases.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:35
+  Expected<StringRef> Wildcard = UnsignedFormat.getWildcardRegex();
+  EXPECT_TRUE(bool(Wildcard));
+  EXPECT_EQ(*Wildcard, "[0-9]+");
----------------
`ASSERT_THAT_EXPECTED(Wildcard, Succeeded());`

Assert because otherwise the next line will crash if it fails, and the _THAT_EXPECTED macro for readability.

Same comment below.

Perhaps worth naming this `UnsignedWildcard` (`HexLowerWildcard` etc), and avoid reusing the variable in the other cases.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:45-46
+  StringRef ALower = bufferize(SM, "a");
+  EXPECT_THAT_ERROR(UnsignedFormat.valueFromStringRepr(ALower, SM).takeError(),
+                    Failed());
+
----------------
I've given this a bit more thought, and I think it would be better here and in similar situations with failing Expecteds to actually check the error contents. You can see examples of this in the DWARFDebugLineTest.cpp unit tests (look for the `checkError` function for a rough guide).

`takeError` on an Expected seems weird without first having checked that the `Expected` actually failed. You probably want `EXPECT_THAT_EXPECTED(someFunctionThatReturnsAnExpected(), Failed());` if you aren't going to actually check the properties of the `Error` within the `Expected`.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:80
+  // Check methods output for NoFormat and Conflict.
+  ExpressionFormat NoneFormat;
+  EXPECT_THAT_ERROR(NoneFormat.getWildcardRegex().takeError(), Failed());
----------------
Test "NoFormat" explicitly by passing it into the constructor and then consider a separate test to show that the default constructor generates a NoFormat kind.

Also `NoneFormat` -> `NoFormat`.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:90
+  EXPECT_FALSE(bool(ConflictFormat));
+  EXPECT_TRUE(bool(UnsignedFormat));
+
----------------
What about the other formats? Why is this here ratehr than in the individual test cases?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:93
+  // Check format equality operators.
+  EXPECT_TRUE(UnsignedFormat == UnsignedFormat);
+  EXPECT_FALSE(UnsignedFormat != UnsignedFormat);
----------------
Self-comparison feels like a special case. You probably want to use a second format instance, I reckon.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:102
+
+  // Check format Vs kind equality operators.
+  EXPECT_TRUE(UnsignedFormat == ExpressionFormat::Kind::Unsigned);
----------------
Vs -> versus


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:212
 
-  // Defined variable: eval returns right value.
+  // Defined variables: eval returns right value.
   Expected<uint64_t> Value = Binop.eval();
----------------
This comment seems stale compared to what is being done below (i.e. it's not just about the `eval()` function it appears on the surface).


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:340
   FileCheckPatternContext Context;
-  Pattern P{Check::CheckPlain, &Context, LineNumber++};
+  Pattern P{Check::CheckPlain, &Context, LineNumber};
 
----------------
This '++' change, and the corresponding changes below, seem unrelated?


================
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]]"));
----------------
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?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:541
+  EXPECT_TRUE(Tester.matchExpect("g"));
+  EXPECT_FALSE(Tester.matchExpect("c"));
+  Tester.initNextPattern();
----------------
does a check against "C" here make sense?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:545
+  EXPECT_TRUE(Tester.matchExpect("H"));
+  EXPECT_FALSE(Tester.matchExpect("B"));
+
----------------
Does a check against "b" here make sense?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:583
+  EXPECT_FALSE(Tester.matchExpect(std::to_string(Tester.getLineNumber())));
+  std::cout << std::to_string(Tester.getLineNumber()) << std::endl;
   Tester.initNextPattern();
----------------
Debug printing left in by mistake?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:744
+                                      /*LineNumber=*/1, &Cxt, SM);
+  ASSERT_TRUE(bool(ExpressionPointer));
+  ExpressionVal = (*ExpressionPointer)->getAST()->eval();
----------------
`ASSERT_THAT_EXPECTED(ExpressionPointer, Succeeded());`
Same all over the place.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389