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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added a comment.

Some minor comments, but generally looking good to me.
We have a few downstream tests that will really benefit from matching Hex upper/lower so looking forward to this landing.



================
Comment at: llvm/lib/Support/FileCheck.cpp:98
+
+  ExpressionFormat Format = LeftFormat ? LeftFormat : RightFormat;
+  if (LeftFormat && RightFormat && LeftFormat != RightFormat)
----------------
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.


================
Comment at: llvm/lib/Support/FileCheck.cpp:232
+    NumericVariable = Context->makeNumericVariable(
+        Name, ExpressionFormat::FormatValue::Unsigned);
     Context->GlobalNumericVariableTable[Name] = NumericVariable;
----------------
Not sure about the `FormatValue` name for the enum. Maybe something like `Kind` or `Type`?
Then this would be `ExpressionFormat::Kind::Unsigned` which I think reads slightly better.


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


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


================
Comment at: llvm/lib/Support/FileCheckImpl.h:45
+    Unsigned,
+    /// Value should be printed as hex number.
+    HexUpper,
----------------
The hex comments don't match the order of the enum.
Since you are documenting all other members,  maybe add `Value should be printed an unsigned decimal number.` to `Unsigned`?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:56
+public:
+  operator bool() {
+    return Value != FormatValue::NoFormat && Value != FormatValue::Conflict;
----------------
This should be explicit. Although we could also remove it and always compare to NoFormat. 


================
Comment at: llvm/lib/Support/FileCheckImpl.h:147
+
+  /// Matching format, i.e. format (e.g. hex upper case letters) to use for the
+  /// value when matching it.
----------------
 I would avoid the i.e+e.g sequence:
`The format to use (e.g. hex upper case letters) when matching the value.`


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389