The FileCheck utility is enhanced to support a --ignore-case
option. This is useful in cases where the output of Unix tools
differs in case (e.g. case not specified by Posix).
Details
- Reviewers
Bigcheese jakehehrlich rupprecht • espindola alexander-shaposhnikov jhenderson MaskRay jdoerfert - Commits
- rG5b5b2fd2b8b4: [FileCheck] Implement --ignore-case option.
rL374538: [FileCheck] Implement --ignore-case option.
rGdfd2b6f07fc4: [FileCheck] Implement --ignore-case option.
rL374339: [FileCheck] Implement --ignore-case option.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Oh, this is painful.
I'm curious - what do you think about some other options, e.g. about extending FileCheck to add support for case insensitiveness,
but maybe smb else might have a better idea
I agree it's painful, and I think a FileCheck option is ultimately the best solution, but as an interim non-terrible solution, how about using tr to convert the od output from upper-case to lower-case:
# RUN: od -t x1 %t | tr '[:upper:]' '[:lower:]' | FileCheck ...
You can also use tr a-z A-Z in theory, but that relies on posix conformity of the od tool, which from experience I don't think we can rely on (e.g. I know that at least one version of od used on the Greendragon buildbots that doesn't allow command-lines to be ordered in somewhat arbitrary ways). I think therefore the character classes might be more reliable.
The command od -t x is used to dump data in hex format. The LIT tests assumes that the hex characters are in lowercase. However, there are also platforms which use uppercase letter. The test cases are changed to accept both formats.
Can you just name the platform? Is it AIX? Please give some examples of its output. IIRC it does not support -t x4.
Please also add -A n to the od command line. POSIX says "The format of the input offset is unspecified."
I think updating FileCheck to allow case-insensitive matchers is a good way to fix this. It may be tricky but it's the right thing to do.
Yeah, I agree that this is painful. I'll have a look at FileCheck because this really feels like the right solution.
I am not on AIX but z/OS USS.
FileCheck has now a new option --ignore-case.
I updated the test cases to use the new option. I touched only test cases which have the lower case/upper case letter problem. I also did not add -A n because some tests also check for the address.
Can you split the FileCheck change into a separate revision? Please add some testing (both unittest and regular test if possible).
Thanks for adding the new option --ignore-case. This will be very useful, though it is not clear from the option name that it uses a lowercase pattern:
size_t Pos = IgnoreCase ? Buffer.find_lower(FixedStr) : Buffer.find(FixedStr);
To make it really --ignore-case, the pattern should also be changed to lowercase. It may be worth splitting the patch into two
- [FileCheck] Add --ignore-case ... and add tests to test/FileCheck/
- Fix od tests with the new FileCheck option.
llvm/lib/Support/FileCheckImpl.h | ||
---|---|---|
2 | bool IgnoreCase = false; and delete the member initializer below. |
I'm pleasantly surprised by how simple that FileCheck change is! You also need to update the FileCheck documentation (see llvm/docs/CommandGuide/FileCheck.rst) for the new option.
Added a test case and documentation for the --ignore-case option.
Removed the changed test cases.
I am not sure which pattern I have missed. The fixed string match uses the lowercase find and the regex match uses the Regex::IgnoreCase flag.
llvm/test/FileCheck/check-ignore-case.txt | ||
---|---|---|
18 | s/5/6/ ? |
llvm/test/FileCheck/check-ignore-case.txt | ||
---|---|---|
46 | Nit: no new line at EOF. |
llvm/test/FileCheck/check-ignore-case.txt | ||
---|---|---|
46 | When I download the raw file and look at it in hex mode, then the last byte is 0x0A. That's a new line at EOF, isn't it? |
Mostly LGTM, just a couple test nits. Thanks for fixing FileCheck!
StringRef::find_lower() is a bad name for something that should really be called StringRef::find_case_insensitive(), which I think is the confusion here.
llvm/test/FileCheck/check-ignore-case.txt | ||
---|---|---|
2 | Can you split up these lines with descriptions of what each one does? e.g. ## Check that ... # RUN: FileCheck ... ## Check that other thing ... # RUN: FileCheck ... | |
6 | Can you add a check that case is ignored for -implicit-check-not? e.g. not FileCheck -ignore-case -implicit-check-not=sTrinG |
llvm/utils/FileCheck/FileCheck.cpp | ||
---|---|---|
3 | Use case-insensitive matching? (To be consistent with the doc). |
llvm/test/FileCheck/check-ignore-case.txt | ||
---|---|---|
2 | independent of case. Or, how about case insensitively? Please also add full stops in these comments. | |
6 | -input-file=%s makes it clear -input-file accepts an argument. I think @jhenderson may prefer --input-file=%s. Binary utilities (almost) exclusively use double-dashed forms now. One bad thing with -input-file is that it is incompatible with POSIX Utility Syntax Guideline 5 (grouped options). Though FileCheck is not a POSIX utility and it does not necessarily abide by the rules... | |
18 | sed /^#/d | |
44 | Consider using a regex for 22. |
Updated test case:
- changed description
- options now use double dash
- replaced sed expression
- added a regex to match column
The test fails on Linux: http://lab.llvm.org:8011/builders/clang-cmake-x86_64-sde-avx512-linux/builds/28537/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Acheck-ignore-case.txt If it takes a while to fix please revert while you investigate.
Also, https://github.com/llvm/llvm-project/commit/dfd2b6f07fc40a190335f580d8a965bbebfe94df looks like you touched ~all lines in docs/CommandGuide/FileCheck.rst and llvm/include/llvm/Support/FileCheck.h Maybe you converted them to windows line endings? If so, please undo that. (Maybe revert and reland with fixed line endings so that the diff for the actual change is readable.)
This change broke tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/19066
I reverted it in r374359. Please run ninja check-all before committing.
As described in http://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git, the right way to checkout the repository on windows is:
% git clone --config core.autocrlf=false https://github.com/llvm/llvm-project.git
This is the second time I've reviewed a change like this without realizing, would appreciate tips if this could be more visible in Phab somehow...
Sorry, I really screwed this up. Not only that I managed to insert an additional character in the regex after my last test but the day before I had trouble with the repository (the COM1.o and COM2.o names on Windows) and cloned the repository again - obviously missing the CR/LF setting I had previously in.
llvm/test/FileCheck/check-ignore-case.txt | ||
---|---|---|
6 | Thanks. I may have overly-strong feelings about the -/-- arguments, but I always find it off-putting seeing single-dash long options :) FWIW, I'm less bothered when it's consistent within a test that only uses executables that don't need to care about single versus double dashes, such as this one. | |
46 | I think this may have been a stale comment for the version I was viewing. At some point, Phabricator said "No new line at end of file" for me. Anyway, the final version you committed doesn't have the issue. |
bool IgnoreCase = false; and delete the member initializer below.