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

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

thopre added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:98
+
+  ExpressionFormat Format = LeftFormat ? LeftFormat : RightFormat;
+  if (LeftFormat && RightFormat && LeftFormat != RightFormat)
----------------
arichardson wrote:
> jhenderson wrote:
> > I don't know about anybody else, but `LeftFormat != NoFormat ? LeftFormat : RightFormat` is more readable to me (and removes the need to hard-code the enum value).
> I agree, checking ` != NoFormat` seems clearer to me rather than relying on NoFormat being zero.
> Alternatively, we could use `Optional<ExpressionFormat>` but that seems like unnecessary overhead.
Changed. Note that ExpressionFormat is a structure so this calls the overloaded boolean operator and does not rely on any hard-coded value.


================
Comment at: llvm/lib/Support/FileCheck.cpp:338
+  // Parse format specifier.
+  size_t FormatSpecEnd = Expr.find(',');
+  if (FormatSpecEnd != StringRef::npos) {
----------------
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.


================
Comment at: llvm/lib/Support/FileCheck.cpp:378
   Expr = Expr.ltrim(SpaceChars);
+  StringRef UseExpr = Expr;
   if (!Expr.empty()) {
----------------
arichardson wrote:
> Where does the following code modify Expr? Is it inside the call to parseNumericOperand?
Yes and parseBinop.


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389