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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Nearly there, from my point of view.



================
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");
----------------
jhenderson wrote:
> 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);
> ```
This doesn't appear to have been done here, only in the bufferized bit below.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:146-147
+  // Wrong casing is not tested because valueFromStringRepr() relies on
+  // StringRef's getAsInteger() which does not allow to restrict casing to
+  // allow.
+  BufferizedInvalidTenStr = bufferize(SM, InvalidTenStr);
----------------
Spurious "to allow"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389