[PATCH] D60384: FileCheck [4/12]: Introduce @LINE numeric expressions

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Another round of commentary nits, plus a couple more substantive remarks.  If you accept them all, LGTM.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:529
 but verify that that register is used consistently later.  To do this,
-:program:`FileCheck` allows named variables to be defined and substituted into
-patterns.  Here is a simple example:
+:program:`FileCheck` support pattern expressions that allows pattern variables
+to be defined and substituted into patterns.  Here is a simple example:
----------------
allow


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:529
 but verify that that register is used consistently later.  To do this,
-:program:`FileCheck` allows named variables to be defined and substituted into
-patterns.  Here is a simple example:
+:program:`FileCheck` support pattern expressions that allows pattern variables
+to be defined and substituted into patterns.  Here is a simple example:
----------------
supports pattern expressions that allow


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:566
 
-Sometimes there's a need to verify output which refers line numbers of the
+Sometimes there's a need to verify output which contains line numbers of the
 match file, e.g. when testing compiler diagnostics.  This introduces a certain
----------------
which -> that


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:574
+``[[#@LINE+<offset>]]`` and ``[[#@LINE-<offset>]]`` numeric expressions in
+patterns, with an arbitrary number of spaces between each elements of the
+expression. These expressions expand to the number of the line where a pattern
----------------
each element


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:592
+variable syntax: ``[[@LINE]]``, ``[[@LINE+<offset>]]`` and
+``[[@LINE-<offset>]]`` without no space inside the brackets and where
+``offset`` is an integer.
----------------
"without any spaces" or "with no spaces"


================
Comment at: llvm/include/llvm/Support/FileCheck.h:108
+  /// Return the result of the substitution represented by this class instance
+  /// or nothing if substitution failed. For a numeric expression we substitute
+  /// it by its value. For a pattern variable we simply replace it by the text
----------------
or None if substitution failed.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:44
+/// Class representing a numeric expression.
+class FileCheckNumExpr {
+private:
----------------
jhenderson wrote:
> thopre wrote:
> > jhenderson wrote:
> > > I don't think this is a strict requirement, but I think it would be worth putting everything in a namespace, to allow these classes to all be called NumericExpression, PatternContext etc without any abbreviations (or with fewer). That would make this code a little more descriptive and less repetitive everywhere.
> > > 
> > > If you did that, you should probably do it in a different change.
> > You mean put all FileCheck classes in a FileCheck namespace?
> Yes, that was what I was referring to (or similar to that anyway). Quite possibly this was discussed before.
+1 for namespace, eventually, although most namespaces are lowercase so I'd prefer 'filecheck'.


================
Comment at: llvm/lib/Support/FileCheck.cpp:241
+    // substitution mode. Numeric variable names have the same restriction as
+    // their pattern variable counterpart.
     if (PatternStr.startswith("[[")) {
----------------
thopre wrote:
> probinson wrote:
> > I thought numeric variables didn't exist until a later patch?
> Regular numeric variables yes but this patch rebrand @LINE as being a pseudo numeric variable and expressions being based on it as being a numeric expression (if of the form [[#<something>]]) or legacy numeric expression.
Right, however the comment is describing the naming rule, and right now the naming rule for numeric variables is "@LINE".
I recognize that when we're done, the comment will be right, but for this patch it is not, and the commentary should describe the code as-it-is and not as-we-wish-it-to-be-later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60384