This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Add support for hex alternate form in FileCheck
ClosedPublic

Authored by thopre on Mar 3 2021, 2:54 AM.

Details

Summary

Add printf-style alternate form flag to prefix hex number with 0x when
present. This works on both empty numeric expression (e.g. variable
definition from input) and when matching a numeric expression. The
syntax is as follows:

[[#%#<precision specifier><format specifier>, ...]

where <precision specifier> and <format specifier> are optional and ...
can be a variable definition or not with an empty expression or not.

This feature was requested in https://reviews.llvm.org/D81144#2075532
for llvm/test/MC/ELF/gen-dwarf64.s

Diff Detail

Event Timeline

thopre created this revision.Mar 3 2021, 2:54 AM
thopre requested review of this revision.Mar 3 2021, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 2:54 AM
jdenny added inline comments.Mar 5 2021, 5:22 PM
llvm/lib/FileCheck/FileCheck.cpp
156

Is there a test for this diagnostic?

jdenny added inline comments.Mar 6 2021, 4:37 AM
llvm/lib/FileCheck/FileCheck.cpp
118–119

AlternateFormPrefix and SignPrefix are reversed relative to the version without leading zeroes below. Can it actually matter for hex? Regardless, it should probably be consistent.

llvm/lib/FileCheck/FileCheckImpl.h
57

Can we have a more exact name than AlternateForm? Maybe HexPrefix. If generality is better, then maybe TypePrefix.

thopre updated this revision to Diff 328953.Mar 8 2021, 2:00 AM
thopre marked 2 inline comments as done.

Address comments

llvm/lib/FileCheck/FileCheck.cpp
118–119

It does not matter because the 0x prefix is only for hex format but I agree better to be consistent.

llvm/lib/FileCheck/FileCheckImpl.h
57

That is taken from printf's manual:

The character % is followed by zero or more of the following flags:

#      The value should be converted to an "alternate form".

For now it only applies to hex values but if we ever support one of the other format e.g. float format) then it would make sense to make it generic. I could rename it for now, we'll need to remember to rename it if a floating-point format gets supported at some point

I've not got the time at the moment to review this or the other recent FileCheck patches. Just wanted to say though that the idea looks good to me.

jdenny added inline comments.Mar 9 2021, 5:02 PM
llvm/docs/CommandGuide/FileCheck.rst
779

The subject is "flag" not "values".

llvm/lib/FileCheck/FileCheck.cpp
131–136

Based on the comments below, it seems the name OverflowErrorStr is misleading. It's a general integer parse error.

152–154

Does this edit say what you mean?

156

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?

llvm/lib/FileCheck/FileCheckImpl.h
57

Ah, thanks, seems like the right name then. Maybe we can have a comment, such as:

/// Is a printf-like "alternate form" specified?
llvm/unittests/FileCheck/FileCheckTest.cpp
262
264
1093

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.

thopre updated this revision to Diff 329642.Mar 10 2021, 6:15 AM
thopre marked 9 inline comments as done.

Address comments

llvm/lib/FileCheck/FileCheck.cpp
131–136

Fixed in D98342

156

Assuming there's no bug in the wildcard regex that indeed should not happen.

llvm/unittests/FileCheck/FileCheckTest.cpp
262

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 patch.

264

Likewise.

1093

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.

jdenny accepted this revision.Mar 10 2021, 2:59 PM

LGTM. Thanks!

llvm/test/FileCheck/numeric-expression.txt
64–65

The above two lines are dead code, right?

llvm/unittests/FileCheck/FileCheckTest.cpp
1093

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.

This revision is now accepted and ready to land.Mar 10 2021, 2:59 PM
thopre added inline comments.Mar 12 2021, 10:13 AM
llvm/test/FileCheck/numeric-expression.txt
64–65

Kind of but I want to have an otherwise valid input for an invalid check file.

This revision was landed with ongoing or failed builds.Mar 12 2021, 10:14 AM
This revision was automatically updated to reflect the committed changes.