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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've still not reviewed the tests in depth yet, but I'm out of time for the day, so here's some more comments from me.



================
Comment at: llvm/include/llvm/Support/FileCheck.h:319
+  /// patterns:
+  /// - tables with the values of live pattern and numeric variables at
+  ///   the start of any given CHECK line;
----------------
thopre wrote:
> jhenderson wrote:
> > tables -> table
> There's 2 tables: one for pattern variables and one for numeric variables. I can split into 2 items if you prefer, I didn't because the comment would be mostly redundant.
Okay, perhaps worth changing it slightly to:

```
- separate tables with the values of live pattern and numeric variables respectively at ... 
```


================
Comment at: llvm/include/llvm/Support/FileCheck.h:96-97
 
-  /// \returns the value being matched against.
-  uint64_t getValue() const { return Value; }
+  /// Evaluates and \returns the value of this numeric expression. Uses
+  /// EvalBinop to perform the binary operation it consists of. \returns None
+  /// if the numeric variable used is undefined.
----------------
Is it legal to have two "\returns" in a doxygen comment? Perhaps it's better to fold the two into one e.g:
```
Evaluates the value of this numeric expression, using EvalBinop to perform the binary operation it consists of. \returns None i the numeric variable used is undefined, or the expression value.
```


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


================
Comment at: llvm/include/llvm/Support/FileCheck.h:233
+  /// patterns. Earlier definition of the variables, if any, have their own
+  /// class instance not referenced from this table.
+  StringMap<FileCheckNumericVariable *> GlobalNumericVariableTable;
----------------
from -> by


================
Comment at: llvm/lib/Support/FileCheck.cpp:647-648
+                                     uint64_t OperandRight) {
+  NumExprs.emplace_back(
+      new FileCheckNumExpr(EvalBinop, OperandLeft, OperandRight));
   return NumExprs.back().get();
----------------
Use `push_back` and `make_unique` here and below.


================
Comment at: llvm/test/FileCheck/numeric-defines.txt:1
+; RUN: FileCheck -D#NUMVAL=12 -check-prefix CHECKNUM -input-file %s %s
+; RUN: not FileCheck -D#NUMVAL=8 -check-prefix CHECKNUM -input-file %s %s 2>&1 \
----------------
I know this isn't necessarily a new issue, but it grates slightly seeing a mixture of single and double dash long options in the same test. Would you mind rationalising it to one or the other (I prefer double, but don't really care). Same goes for the other tests.


================
Comment at: llvm/test/FileCheck/numeric-defines.txt:24
+
+; NUMERRCLIFMT: Global defines:1:20: error: invalid name in numeric variable definition '10VALUE'
+; NUMERRCLIFMT-NEXT: Global define #1: #10VALUE=10
----------------
Thanks for splitting things up. I'd go one step further and split the bad format tests into a separate test from the good format ones, to make the test easier to follow. I'd also add comments to the start of the test files to clearly describe what the test is intended to actually test.


================
Comment at: llvm/test/FileCheck/pattern-defines.txt:1
 ; RUN: FileCheck -DVALUE=10 -input-file %s %s
+; RUN: not FileCheck -DVALUE=20 -input-file %s %s 2>&1 \
----------------
Similar to elsewhere, it would be nice for some comments explaining the purpose of this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60385