[PATCH] D97845: [FileCheck] Add support for hex alternate form in FileCheck

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

thopre added inline comments.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:152
+  if (MissingFormPrefix)
+    return ErrorDiagnostic::get(SM, StrVal, "missing alternate form prefix");
+
----------------
jdenny wrote:
> jdenny wrote:
> > Is there a test for this diagnostic?
> Thanks for the unit test.
> 
> Is this error possible within the normal FileCheck utility?  That is, if the regex matched, then the match must have the prefix, right?
Assuming there's no bug in the wildcard regex that indeed should not happen.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:131
   bool ValueIsSigned = Value == Kind::Signed;
   StringRef OverflowErrorStr = "unable to represent numeric value";
+
----------------
jdenny wrote:
> Based on the comments below, it seems the name `OverflowErrorStr` is misleading.  It's a general integer parse error.
Fixed in [[ https://reviews.llvm.org/D98342 | D98342 ]]


================
Comment at: llvm/unittests/FileCheck/FileCheckTest.cpp:263
+    LongNumberStr = addBasePrefix(RefusedHexOnlyDigits);
     checkWildcardRegexCharMatchFailure(RefusedHexOnlyDigits);
   }
----------------
jdenny wrote:
> 
It's the addBasePrefix that should be removed because the goal is to check that every characters passed to checkWildcardRegexCharMatchFailure are rejected so the addBasePrefix is done inside the function for each of the characters. In retrospect checkWildcardRegexCharMatchFailure ought to take a vector instead of a string. I've fixed it in a separate [[ https://reviews.llvm.org/D98343 | patch ]].


================
Comment at: llvm/unittests/FileCheck/FileCheckTest.cpp:266
+  LongNumberStr = addBasePrefix(FirstInvalidCharDigits);
   checkWildcardRegexCharMatchFailure(FirstInvalidCharDigits);
 
----------------
jdenny wrote:
> 
Likewise.


================
Comment at: llvm/unittests/FileCheck/FileCheckTest.cpp:1095
+  expectDiagnosticError("alternate form only supported for hex values",
+                        Tester.parseSubst("%#d, PREFIXED_UNSI:").takeError());
+
----------------
jdenny wrote:
> How do you decide what cases to test here for the FileCheck library but not for the FileCheck utility?  For example, I didn't find these cases tested for the utility.  I'm not saying that's wrong.  I'm just trying to figure out what the right approach is.  I feel like we've discussed this before, but I've forgotten.
I'm using llvm/test/FileCheck are for user-level functionality and llvm/unittests/FileCheck for getting code coverage for individual functions. Obviously that creates some overlap (e.g. where to test error messages?) which I try to avoid by keeping details only in the unittesting. Any error that is important from a user point of view is likely to be tested twice though.

There might be some inconsistencies to that rule because I used to test function-level interface in unittest rather than focus on code coverage. I've tried to solve all of those in https://reviews.llvm.org/D72912 but I might have missed some.

For this specific error I probably should test it in numeric-expressions.txt as well (hence now done). Let me know if you disagree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97845