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

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

thopre 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.
+
----------------
jhenderson wrote:
> Perhaps worth clarifying that an empty constraint means any value is accepted.
I don't understand what you mean. As explained here an empty constraint is the same as the == constraint and means an exact match, as in [[#FOO]] with FOO equal to 10 will only match 10.  Or do you mean for variable substitution with empty numeric expression? The paragraph for such case is above:

The syntax to define a numeric variable is ``[[#%<fmtspec>,<NUMVAR>:]]`` where:

* ``%<fmtspec>`` is an optional scanf-style matching format specifier to
  indicate what number format to match (e.g. hex number).  Currently accepted
  format specifiers are ``%u``, ``%d``, ``%x`` and ``%X``.  If absent, the
  format specifier defaults to ``%u``.

* ``<NUMVAR>`` is the name of the numeric variable to define to the matching
  value.

For example:

.. code-block:: llvm

    ; CHECK: mov r[[#REG:]], 0x[[#%X,IMM:]]

would match ``mov r5, 0xF0F0`` and set ``REG`` to the value ``5`` and ``IMM``
to the value ``0xF0F0``.


================
Comment at: llvm/lib/Support/FileCheck.cpp:602
+  Expr = Expr.ltrim(SpaceChars);
+  bool MaybeInvalidConstraint = true;
+  if (Expr.consume_front("=="))
----------------
jhenderson wrote:
> 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.
How about HasParsedValidConstraint? If nothing is parsed the variable remains false which does not contradict the fact that the default constraint applies and is valid. As to the parseNumericOperand function it is sometimes called in a context where the text being parsed cannot be an invalid constraint (e.g. when called from parseBinop for the second operand) and thus we only want to warn about invalid operand, yet no constraint has been parsed. This is why I used MaybeInvalidConstraint. If true, then we should warn about invalid operand format or constraint, if not we should only warn about invalid operand format.


================
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
----------------
jhenderson wrote:
> //\ -> ///
> 
> 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).
See above why I did not rename the parameter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60391