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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny accepted this revision.
jdenny added a comment.
This revision is now accepted and ready to land.

LGTM.  Thanks!



================
Comment at: llvm/test/FileCheck/numeric-expression.txt:64-65
+RUN:   | FileCheck --check-prefix INVALID-ALT-FORM-MSG2 --strict-whitespace %s
+DEF INVALID ALT FORM
+PREFIXED_DEC=0x3
+INVALID-ALT-FORM-LABEL: DEF INVALID ALT FORM
----------------
The above two lines are dead code, right?


================
Comment at: llvm/unittests/FileCheck/FileCheckTest.cpp:1095
+  expectDiagnosticError("alternate form only supported for hex values",
+                        Tester.parseSubst("%#d, PREFIXED_UNSI:").takeError());
+
----------------
thopre wrote:
> 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.
I'm not familiar enough with the users of the FileCheck library to know what the appropriate level of testing is in the unit tests.  However, I think it's important to be sure that the FileCheck utility behaves correctly (end-to-end) for all its use cases, and that's hard to guarantee when only testing the library directly.  So, thanks for the new tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97845