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

Thu May 2 03:08:21 PDT 2019

jhenderson added a comment.

(sorry for some duplicated comments, @arichardson posted his whilst I was writing mine).

Not looked at the tests in detail yet, but there's plenty to get on with.

================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:621
+To support this case, FileCheck understands the ``@LINE`` pseudo numeric
+variable which evaluate to the line number of the CHECK pattern where it is
+found.
----------------
evaluate -> evaluates

================
Comment at: llvm/include/llvm/Support/FileCheck.h:54
+  /// represented by \p AST.
+  FileCheckNumExpr(FileCheckASTBinop *AST) : AST(std::move(AST)) {}
+
----------------
probinson wrote:
> 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>` ?
It's not particularly clear here that the FileCheckNumExpr gets ownership of the AST pointer. As a result, it could very easily lead to multiple frees or similar issues. An obvious example would be:

```
{
  FileCheckASTBinOp Op(...);
  FileCheckNumExpr(&Op);
} // Op gets destroyed twice.
```

Assuming that it does need ownership (as implied by the use of unique_ptr), perhaps it would be better to construct the AST instance within the FileCheckNumExpr constructor or some helper method that returns a FileCheckNumExpr instance?

================
Comment at: llvm/include/llvm/Support/FileCheck.h:63
+/// numeric expression.
+class FileCheckNumVar {
+private:
----------------
NumVar (and to a lesser extent NumExpr) is hard to interpret, so probably shouldn't be overly abbreviated. NumericVar or NumericVariable (I'm happy with either) would be better, I think.

================
Comment at: llvm/include/llvm/Support/FileCheck.h:68
+
+  /// Value of numeric variable if defined or None otherwise.
+  llvm::Optional<uint64_t> Value;
----------------
... if defined ... -> ..., if defined, ...

================
Comment at: llvm/include/llvm/Support/FileCheck.h:76
+
+  /// Return name of that numeric variable.
+  StringRef getName() const { return Name; }
----------------
Here and elsewhere, you should probably use the doxygen style for "Return".

================
Comment at: llvm/include/llvm/Support/FileCheck.h:99
+  /// Left operand.
+  FileCheckNumVar *Opl;
+
----------------
Why not call this Left, and the other Right (or LeftOp and RightOp if you want Op to be in the name)? The meaning is then implicit and the comment unnecessary.

================
Comment at: llvm/include/llvm/Support/FileCheck.h:113
+  /// Evaluate the value of the binary operation represented by this AST. Uses
+  /// EvalBinop to perform the binary operation.
+  llvm::Optional<uint64_t> eval() const;
----------------
You should explain under what situations None can be returned here.

================
Comment at: llvm/include/llvm/Support/FileCheck.h:255

+  /// Vector holding pointers to all numeric variables parsed. Used to
+  /// automatically free them once they are guaranteed to no longer be used.
----------------
numeric variables parsed -> parsed numeric variables

================
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;
----------------
tables -> table

================
Comment at: llvm/lib/Support/FileCheck.cpp:43
+  llvm::Optional<uint64_t> Opl = this->Opl->getValue();
+  // Variable has been undefined.
+  if (!Opl)
----------------
"Variable is undefined". (?) It doesn't really matter whether it was upfront undefined or later undefined.

================
Comment at: llvm/lib/Support/FileCheck.cpp:124-125

+static uint64_t doAdd(uint64_t Opl, uint64_t Opr) { return Opl + Opr; }
+static uint64_t doSub(uint64_t Opl, uint64_t Opr) { return Opl - Opr; }
+
----------------
You can probably just name these "add" and "sub".

================
Comment at: llvm/lib/Support/FileCheck.cpp:140
+  // CHECK pattern. For each definition, the pointer to the corresponding AST
+  // class instance is stored in GlobalNumericVariableTable. Therefore the
+  // pointer we get below is for the AST class instance corresponding to the
----------------
Therefore, the

================
Comment at: llvm/lib/Support/FileCheck.cpp:150
+
+  FileCheckNumVar *Opl = VarTableIter->second;
+
----------------
LeftOp

================
Comment at: llvm/lib/Support/FileCheck.cpp:182
   }
-  uint64_t Offset;
-  if (Trailer.consumeInteger(10, Offset)) {
+  uint64_t Opr;
+  if (Trailer.consumeInteger(10, Opr)) {
----------------
RightOp

================
Comment at: llvm/lib/Support/FileCheck.cpp:216
+  uint64_t LineNumber64 = LineNumber;
+  auto LinePseudoVar = Context->makeNumVar(LinePseudo, LineNumber64);
+  Context->GlobalNumericVariableTable[LinePseudo] = LinePseudoVar;
----------------
This should probably not be `auto` (is it a pointer? What is the type etc).

================
Comment at: llvm/lib/Support/FileCheck.cpp:1689
+  // Numeric expression substitution reads the value of a variable directly,
+  // not via GlobalNumericVariableTable. Therefore we clear local variables by
+  // clearing their value which will lead to a numeric expression substitution
----------------
Therefore, we

================
Comment at: llvm/test/FileCheck/defines.txt:15
+
+; 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 | FileCheck %s --strict-whitespace -check-prefix NUMERRMSG
----------------
The test is getting quite dense. It might be worth either a) splitting it up, or b) labelling each sub-test with a comment.

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:2
+; RUN: FileCheck -D#VAR1=11 -input-file %s %s
+; RUN: not FileCheck -check-prefix UNDEF-USE -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix UNDEF-USE-MSG %s
+; RUN: not FileCheck -D#VAR1=11 -check-prefixes CHECK,INVAL-OP -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix INVAL-OP-MSG %s
----------------
It may be worth splitting this (and the other cases) over multiple lines like this:

```
; RUN: not FileCheck -check-prefix UNDEF-USE -input-file %s %s 2>&1 \
; RUN:   | FileCheck --strict-whitespace -check-prefix UNDEF-USE-MSG %s
```

I also would find it easier to read if you grouped the runs with their corresponding checks:

```
; RUN: FileCheck... --check-prefix=FOO

; FOO: blah
; FOO-NEXT: blahblah

; RUN: FileCheck... --check-prefix=BAR

; BAR: bar
...

```

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:7
+; RUN: not FileCheck -D#VAR1=11 -DPATVAR=foobar -D#PATVAR=42 -check-prefix CONFLICT -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix CLI-CLI-NUM-CONFLICT %s
+
+
----------------
No need for double blank lines here and below.

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:9
+
+; We ensure we attemt to match all lines with digits by using CHECK-NEXT
+; directives for the checks to ensure each line of input is covered.
----------------
attemt -> attempt

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:10
+; We ensure we attemt to match all lines with digits by using CHECK-NEXT
+; directives for the checks to ensure each line of input is covered.
+
----------------
Doesn't the second "ensure" just repeat what you said in the first part of the sentence?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60385