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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 10 06:32:02 PDT 2020


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.
+
----------------
thopre wrote:
> jhenderson wrote:
> > thopre wrote:
> > > 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``.
> > So on further review of the doc as a whole, I think there's some redundancy involved. This is actually all one single syntax, yet is split into "numeric definitions" and "numeric substitutions", and then further "definitions with constraints" which is actually just the unification of the two.
> > 
> > Not for this patch, but I think we can reduce the documentation by starting with the unified syntax (`[#%<fmtspec>,<NUMVAR>:<constraint>]`) and breaking down each part individually:
> > 
> > - `#` is required to mark this as a numeric expression as opposed to a string variable use/definition.
> > - `%<fmtspec>` is optional and indicates the format of the variable to be matched. If not specified, the format is derived from the constrain (if present), or inferred from the matched string (or whateer the rule actually is).
> > - `,` is there iff both `%<fmtspec>` and either `<NUMVAR>:` or `<constraint>` are.
> > - `<NUMVAR>:` specifies variable name being defined. Optional, so if not present, no variable is defined.
> > - `<constraint>` if specified limits what this substitution can match to. If not present, anything matched by the `%<fmtspec>` is permitted.
> > 
> > Everything else just falls out naturally - a definition without any other parts is a definition without constraint. A constraint without a definition just limits what the matched number can be etc. It doesn't hurt to give examples of each of these sub-cases, but I'd start with the combined case and break it down.
> Funny you should mention that because the documentation used to be like that (albeit without many cleanup that have happen since) and was changed in this split up version. See https://reviews.llvm.org/D49084#1338039.
> 
> The directives are also structured like this somewhat with first directives not mentioning variables and or regex. Things get introduced progressively to ease the reading. One thing that could be done is give the full syntax and then break it up into definition and expression, ending with a mixed of the two. That seems to be what you suggest.
Yes, I think it's a good idea to start with the combined syntax, then split it up, rather than the other way around. That way, it's clear from the start that everything is one single syntax, just with different ways of using it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60391





More information about the llvm-commits mailing list