[PATCH] D60385: FileCheck [5/12]: Introduce regular numeric variables

Wed May 1 14:14:21 PDT 2019

probinson added a comment.

Haven't looked at the tests yet.

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:110
+
+  Sets a filecheck numeric variable ``VAR`` to ``<VALUE>`` that can be used
+  in ``CHECK:`` lines.
----------------
VAR -> NUMVAR

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:571
+
+:program:`FileCheck` also allows to check for numeric values satisfying a
+numeric expression constraint based on numeric variables. This allows to
----------------
"allows checking for"
"values that satisfy a"

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:573
+numeric expression constraint based on numeric variables. This allows to
+capture numeric relations between two numbers in a text to check, such as the
+the need for consecutive registers to be used.
----------------
"This allows CHECK: directives to verify a numeric relation between two numbers, such as "

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:574
+capture numeric relations between two numbers in a text to check, such as the
+the need for consecutive registers to be used.
+
----------------
'the' appears at the end of the previous line and again at the start of this line.

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:594
+
+The above example would match the lines:
+
----------------
lines -> line

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:600
+
+but would not match the lines:
+
----------------
lines -> line

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:608
+
+In the same way as for pattern variables, ``--enable-var-scope`` only consider
+global numeric variables that start with ``$`` and undefined local variables at
----------------
"The ``--enable-var-scope`` option has the same effect on numeric variables as on pattern variables."

(No need to repeat any more of the description here.)

================
Comment at: llvm/include/llvm/Support/FileCheck.h:68-72
+  /// Whether variable is defined and thus Value is set.
+  bool Defined;
+
+  /// Value of numeric variable if defined.
   uint64_t Value;
----------------
arsenm wrote:
> It seems like you're just recreating Optional here, and then figure out how to return it later. You can just use Optional<uint64_t> as the member
I agree with Matt, this should be Optional<uint64_t> Value, it will simplify things.

================
Comment at: llvm/include/llvm/Support/FileCheck.h:54
+  /// represented by \p AST.
+  FileCheckNumExpr(FileCheckASTBinop *AST) : AST(std::move(AST)) {}
+
----------------
I don't claim to be an expert on unique_ptr and move, but doing a move from a raw pointer looks strange. Perhaps the parameter should be a `std::unique_ptr<FileCheckASTBinop>` ?

================
Comment at: llvm/include/llvm/Support/FileCheck.h:77
+  /// time (e.g. the @LINE numeric variable).
+  explicit FileCheckNumVar(StringRef Name, uint64_t Value)
+      : Name(Name), Defined(true), Value(Value) {}
----------------
This now has two parameters so it does not need to be 'explicit'.

================
Comment at: llvm/include/llvm/Support/FileCheck.h:269
+  /// Define pattern and numeric variables from definitions given on the
+  /// command line, passed as a vector of VAR=VAL strings in \p CmdlineDefines.
+  /// Report any error to \p SM and return whether an error occured.
----------------
"vector of [#]VAR=VAL strings"

================
Comment at: llvm/lib/Support/FileCheck.cpp:33
+
+bool FileCheckNumVar::setValue(uint64_t Value) {
+  if (Defined)
----------------
You could name the parameter NewValue then you don't need to qualify members with `this->`

================
Comment at: llvm/lib/Support/FileCheck.cpp:46
+  return false;
+}
+
----------------
The above 3 functions are all simplified if Value is an Optional.

================
Comment at: llvm/lib/Support/FileCheck.cpp:81
+    // Although use of undefined numeric variable is tested at parse time,
+    // numeric variable can get undefined later by ClearLocalVariables.
+    return NumExpr->getAST()->getUndefVarName();
----------------
"Although a use of an undefined numeric variable is detected at parse time, a numeric variable can be undefined later by ClearLocalVariables."

================
Comment at: llvm/lib/Support/FileCheck.cpp:83
+    return NumExpr->getAST()->getUndefVarName();
+  else {
+    if (!Context->getPatternVarValue(FromStr))
----------------
Don't use 'else' after 'return' (long-standing LLVM style).

================
Comment at: llvm/lib/Support/FileCheck.cpp:147
+  // This method is indirectly called from ParsePattern for all numeric
+  // variable definition and uses in the order in which they appear in the
+  // CHECK pattern. For each definition, the pointer to the corresponding AST
----------------
definition -> definitions

================
Comment at: llvm/lib/Support/FileCheck.cpp:161
+
+  // Check if this is a supported operation and selection function to perform
+  // it.
----------------
selection -> select a

================
Comment at: llvm/lib/Support/FileCheck.cpp:1678
+      GlobalVariableTable.insert(CmdlineNameVal);
+      // Mark pattern variable as defined to detect collision between pattern
+      // and numeric variable in DefineCmdlineVariables when the latter is
----------------
"Mark the "
collision -> collisions

================
Comment at: llvm/lib/Support/FileCheck.cpp:1679
+      // Mark pattern variable as defined to detect collision between pattern
+      // and numeric variable in DefineCmdlineVariables when the latter is
+      // created later than the former. We cannot reuse GlobalVariableTable for
----------------
variable -> variables

================
Comment at: llvm/lib/Support/FileCheck.cpp:1681
+      // created later than the former. We cannot reuse GlobalVariableTable for
+      // that by populating it with an empty string since we would then loose
+      // the ability to detect use of undefined variable in Match().
----------------
loose -> lose

================
Comment at: llvm/lib/Support/FileCheck.cpp:1682
+      // that by populating it with an empty string since we would then loose
+      // the ability to detect use of undefined variable in Match().
+      DefinedVariableTable[Name] = true;
----------------
"detect the use of an undefined"

================
Comment at: llvm/lib/Support/FileCheck.cpp:1696

+  // Numeric expression substitution read the value of variable directly, not
+  // via GlobalNUmericVariableTable. Therefore we clear local variable by
----------------
"reads the value of a variable"

================
Comment at: llvm/lib/Support/FileCheck.cpp:1697
+  // Numeric expression substitution read the value of variable directly, not
+  // via GlobalNUmericVariableTable. Therefore we clear local variable by
+  // clearing their value which will lead to an numeric expression substitution
----------------
NU -> Nu
variable -> variables

================
Comment at: llvm/lib/Support/FileCheck.cpp:1698
+  // via GlobalNUmericVariableTable. Therefore we clear local variable by
+  // clearing their value which will lead to an numeric expression substitution
+  // failure. We also mark the variable for removal from
----------------
an -> a

================
Comment at: llvm/lib/Support/FileCheck.cpp:1749
+    // Do not clear the first region as it's the one before the first
+    // CHECK-LABEL and it would clear variable defined on the command-line
+    // before they get used.
----------------
variable -> variables

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60385