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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheckImpl.h:40
+    /// empty expressions.
+    NoFormat = 0,
+    /// Used when there are several conflicting implicit formats in an
----------------
Am I right in thinking you can get rid of the `= 0` now?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:56
+public:
+  /// Evaluates a format to true if it can be matched.
+  explicit operator bool() {
----------------
This comment isn't clear to me. I assume what it means by "if it can be matched" is "if it is a format that can be used in a match" ot something similar?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:62
+  /// Define format equality: formats are equal if not NoFormat and their kind
+  /// are same.
+  bool operator==(const ExpressionFormat &Other) {
----------------
if neither is NoFormat and their kinds are the same.


================
Comment at: llvm/test/FileCheck/numeric-defines.txt:4
 
-RUN: FileCheck -D#NUMVAL1=8 -D#NUMVAL2='NUMVAL1 + 4' -check-prefixes CHECKNUM1,CHECKNUM2 -input-file %s %s
+# Tests with default format specifier
+RUN: FileCheck -D#NUMVAL1=8 -D#NUMVAL2='NUMVAL1 + 4' --check-prefixes CHECKNUM1,CHECKNUM2 --input-file %s %s
----------------
Nit: trailing full stop.

You're also mixing your comment characters. Here you use '#', but two lines above you use ';'.


================
Comment at: llvm/test/FileCheck/numeric-defines.txt:50
+
+
+; Tests with explicit format specifiers.
----------------
Nit: too many blank lines (1 is enough)


================
Comment at: llvm/test/FileCheck/numeric-defines.txt:54
+
+RUN: %ProtectFileCheckOutput \
+RUN: not FileCheck -D#%X,NUMVAL3=7 -D#%X,NUMVAL4=12 --check-prefix CHECKNUM3 --input-file %s %s 2>&1 \
----------------
I'm not familiar with this "%ProtectFileCheckOutput". What is it for, and why do only some cases seem to use it?


================
Comment at: llvm/test/FileCheck/numeric-defines.txt:73
+; Test with explicit and implicit format specifiers.
+RUN: FileCheck -D#%X,NUMVAL3=8 -D#NUMVAL4='NUMVAL3 + 4' --check-prefixes CHECKNUM3,CHECKNUM4 --input-file %s %s
+
----------------
You should probably also have this case where the format is on the second value, and not the first.


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:173
+; lines.
+USE CONV FMT IMPL MATCH
+b
----------------
Aside from being fewer in number, how is this set different from the "USE DEF FMT IMPL MATCH" set?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:43
+  EXPECT_EQ(HexLowerFormat.getMatchingString(11), "b");
+  Val = HexLowerFormat.valueFromStringRepr(ALower, SM);
+  ASSERT_TRUE(bool(Val));
----------------
It might be interesting to show what happens when you pass an upper-case hex digit to a lower-case format and vice versa.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:60
+
+#ifndef NDEBUG
+  ExpressionFormat NoneFormat;
----------------
What's going on here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60389