[PATCH] D60386: FileCheck [6/12]: Introduce numeric variable definition

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've barely scratched the surface of this so far, but here are some initial comments for where I've got to.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:576
+
+The syntax to define a numeric variable is ``[[#<NUMVAR>:]]`` where ``NUMVAR``
+is the name of the numeric variable to define to the matching value.
----------------
Since an example usage is actually `[[#FOO:]]`, either lose the '<' and '>' in the first bit, or add them in the second.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:590
 
-* ``<NUMVAR>`` is the name of a numeric variable defined on the command line.
+* ``<NUMVAR>`` is the name of a numeric variable defined on a previous line.
 
----------------
Perhaps worth adding "or on the command line".


================
Comment at: llvm/include/llvm/Support/FileCheck.h:45
+/// expression. Each definition of a variable gets its own instance of this
+/// class, variable uses share the same instance as the respective definition.
 class FileCheckNumericVariable {
----------------
class, variable -> class. Variable


================
Comment at: llvm/include/llvm/Support/FileCheck.h:56
+  /// variable is defined on the same line as a given use.
+  unsigned DefLineNumber;
+
----------------
I'd be inclined to make this size_t rather than unsigned, as it's tied to the file system/memory availability. (However, see the note below).

Probably this should be assigned to 0 for command-line defined variables?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:60-61
+  /// Constructor for a variable \p Name defined at line \p DefLineNumber.
+  FileCheckNumericVariable(StringRef Name, unsigned DefLineNumber)
+      : Name(Name), Value(llvm::None), DefLineNumber(DefLineNumber) {}
+
----------------
It is theoretically possible for a standards-compliant compiler to find this and the next constructor ambiguous, because there is nothing stopping unsigned to be the same size as uint64_t. This becomes even more likely for size_t versus uint64_t. Furthermore, a user could very easily call the wrong constructor unintentionally.

I'd therefore suggest changing the signature somehow so as to avoid the risk of confusion for users of this class. Options include writing a pair of factory methods (effectively named constructors), or reordering the signature of one. There may be other options too.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:145
 public:
-  /// Constructor for a pattern variable substitution.
+  /// Constructor for a pattern substitution.
   FileCheckPatternSubstitution(FileCheckPatternContext *Context,
----------------
I'm not sure I understand why these comments have changed?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:225
+/// Structure representing the definition of a numeric variable in a pattern.
+/// It holds the parenthesized capture number and the pointer to the class
+/// representing the numeric variable whose value is being defined.
----------------
What is a "capture number"? It probably needs explaining.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:232
+
+  /// Parenthesized capture number for this numeric variable definition.
+  unsigned CaptureParen;
----------------
See above comment. I have no idea what a "capture number" is or why it needs parenthesizing.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:256
   /// representing the last definitions of numeric variables defined in
   /// previous patterns. Earlier definition of the variables, if any, have
+  /// their own class instance not referenced from this table. When matching a
----------------
definition -> definitions.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:257
   /// previous patterns. Earlier definition of the variables, if any, have
-  /// their own class instance not referenced by this table.
+  /// their own class instance not referenced from this table. When matching a
+  /// pattern all definitions for that pattern are recorded in
----------------
from -> by (as it was).


================
Comment at: llvm/include/llvm/Support/FileCheck.h:259
+  /// pattern all definitions for that pattern are recorded in
+  /// NumericVariableDefs table.
   StringMap<FileCheckNumericVariable *> GlobalNumericVariableTable;
----------------
What is "NumericVariableDefs" referring to here?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:332
 
+  /// Holds the capture parentheses number and pointer to corresponding
+  /// FileCheckNumericVariable class instance of all numeric variable
----------------
See above comments re. capture parentheses

pointer to corresponding -> pointer to the corresponding


================
Comment at: llvm/include/llvm/Support/FileCheck.h:335
+  /// definitions. Used to set the matched value of all those variables.
+  std::map<StringRef, FileCheckNumExprMatch> NumericVariableDefs;
+
----------------
Should this use a StringMap?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:354
+                            FileCheckPatternContext *Context,
+                            unsigned LineNumber)
+      : Context(Context), CheckTy(Ty), LineNumber(LineNumber) {}
----------------
I'd change this name to `Line`, to avoid any risk of clashes with the member.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:380
+  /// representing the AST of the numeric expression whose value must be
+  /// substituted or nullptr if parsing fails, in which case errors are
+  /// reported on \p SM. Sets \p DefinedNumericVariable to point to the class
----------------
I'd put a comma after "substituted"


================
Comment at: llvm/include/llvm/Support/FileCheck.h:442
+  /// \returns the class representing that binary operation of the numeric
+  /// expression or nullptr if parsing fails in which case errors are reported
+  /// on \p SM.
----------------
Commas after "expression" and "fails".

The whole first part of this sentence feels clunky. I think it's the use of "that" that's the problem. Probably just replace with "the".


================
Comment at: llvm/include/llvm/Support/FileCheck.h:444
+  /// on \p SM.
+  FileCheckNumExpr *parseFileCheckBinop(StringRef &Expr,
+                                        const SourceMgr &SM) const;
----------------
Could this just be `parseBinop`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60386