[PATCH] D60391: FileCheck [11/12]: Add matching constraint specification

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:692-693
+  accepted constraint is ``==`` for an exact match and is the default if
+  ``<constraint>`` is not provided. No matching constraint must be specified
+  when the ``<expr>`` is empty.
+
----------------
Perhaps worth clarifying that an empty constraint means any value is accepted.


================
Comment at: llvm/lib/Support/FileCheck.cpp:602
+  Expr = Expr.ltrim(SpaceChars);
+  bool MaybeInvalidConstraint = true;
+  if (Expr.consume_front("=="))
----------------
I think I'd change this variable (also in the function it's used in) to be `HasValidConstraint` and flip the logic, i.e. `HasValidConstraint` is false by default, but set to true if there is a recognised constraint.


================
Comment at: llvm/lib/Support/FileCheck.cpp:612
+          SM, Expr,
+          "Empty numeric expression should not have a matching constraint");
+  } else {
----------------
The term "matching" both here and in the comment above is a little bit overloaded, since I immediately thought "what is the constraint matching" when in fact it's the expression that is matching something. I'd just delete the term "matching" from all cases referring to the constraint, including the docs. I think "constraint" on its own is sufficient.

Also "Empty" -> "empty"


================
Comment at: llvm/lib/Support/FileCheckImpl.h:716
   /// values and numeric variables, depending on the value of \p AO. Parameter
-  /// \p Context points to the class instance holding the live string and
-  /// numeric variables. \returns the class representing that operand in the
-  /// AST of the expression or an error holding a diagnostic against \p SM
-  /// otherwise. If \p Expr starts with a "(" this function will attempt to
-  /// parse a parenthesized expression.
+  //\ \p MaybeInvalidConstraint indicate whether the text being parsed could be
+  /// an invalid matching constraint. Parameter \p Context points to the class
----------------
//\ -> ///

indicate -> indicates

Obviously this comment will need updating when you switch to `HasValidConstraint`.

I think you can also simplify any "Parameter \p" instances in this comment to simply "\p", since it's obvious from the context what they are referring to (\p is for parameters).


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:125
+; Numeric expressions in default matching format and explicit matching rule using
+; variables defined on other lines
+USE DEF FMT EXPL MATCH
----------------
Nit: missing full stop.


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:133
+; CHECK-NEXT: [[# == UNSI]]
+; CHECK-NEXT: [[#UNSI2: == UNSI + 1]]
+
----------------
It would probably be sensible to confirm using `UNSI2` works as expected, with an extra check.

I might be missing it, but do you have any test cases where an explicit constraint (as opposed to an implicit one) doesn't match?


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:136
+; Empty numeric expression with matching constraint.
+RUN: not FileCheck --check-prefix EMPTY-NUMEXPR-CONSTRAINT --input-file %s %s 2>&1 \
+RUN:   | FileCheck --check-prefix EMPTY-NUMEXPR-CONSTRAINT-MSG --strict-whitespace %s
----------------
Doesn't this need a `ProtectFileCheckOutput` line?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:1029
+  expectDiagnosticError("invalid matching constraint or operand format",
+                        Tester.parseSubst(">=FOO").takeError());
+
----------------
Up to you, but since we'd probably want to support `>=` at some point in the future, I think it might be best for this to pick something that is definitely not a valid constraint, e.g. `+=`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60391