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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:338
+  // Parse format specifier.
+  size_t FormatSpecEnd = Expr.find(',');
+  if (FormatSpecEnd != StringRef::npos) {
----------------
thopre wrote:
> arichardson wrote:
> > Instead of checking for a comma (which be allowed to appear after the ` :` in the future, I would check if the next non-whitespace character is a `%`. Or to simplify this we could require the % to immediately follow the # character?
> I'm not sure I follow the intent of your message, are you against the syntax (i.e. there should be no comma at all) or the way it is parsed? Anyway, since the format comes first, I'm not sure how allowing a comma in the later part of the syntax would be a problem. If I remember well its use in the syntax was suggested to match the API of printf/scanf. Otherwise we'd have:
> [[#%d FOOBAR:N+1]] which I find less easy to read.
Sorry about the ambiguity. The current syntax is perfectly fine.

I was suggesting a change to the parsing code to check that the first non-whitespace char is a `%` first instead of searching for a comma. But this would only change the which error messages for certain invalid input so it shouldn't really matter.




================
Comment at: llvm/test/FileCheck/numeric-defines.txt:19
 RUN: %ProtectFileCheckOutput \
-RUN: not FileCheck -D#NUMVAL1=7 -D#NUMVAL2=12 -check-prefix NUMNOT -input-file %s %s 2>&1 \
-RUN:   | FileCheck %s --strict-whitespace -check-prefixes NOT-NUMERRMSG2
-RUN: FileCheck -D#NUMVAL1=7 -D#NUMVAL2=8 -check-prefixes NUMNOT -input-file %s %s
+RUN: not FileCheck -D#%X,NUMVAL1=7 -D#%X,NUMVAL2=12 --check-prefix NUMNOT --input-file %s %s 2>&1 \
+RUN:   | FileCheck %s --strict-whitespace --check-prefixes NOT-NUMERRMSG2
----------------
thopre wrote:
> arichardson wrote:
> > I find this slightly surprising. If I define a numeric variable on the command line with hex format, I would expect the value to be parsed as a hex number, i.e. 0x12 and not 0xc.
> I didn't think of that. What would be your expectation for implicit format though?
> 
> E.g. -D#%X,NUMVAL1=7 -D#NUMVAL2=12+NUMVAL1. Should NUMVAL2 be 0x17 or decimal 17?
I'm not entirely sure what the best solution there is. 
I think NUMVAL2 should always evaluate to 19.
However, I don't have a strong preference whether it should capture decimal 19 or hex 0x13 (i.e. parse the number in the expresssion as decimal, but inherit format from the other variable).

Alternatively we could require an explicit format for those (probably rare?) cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389