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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
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>` ?
Since the FileCheckNumExpr takes ownership of the pointer, I think the argument should be `std::unique_ptr`


================
Comment at: llvm/include/llvm/Support/FileCheck.h:99
+  /// Left operand.
+  FileCheckNumVar *Opl;
+
----------------
I would name this `OpL`/`OpLeft` or `LHS` since the lowercase l in Opl doesn't make the "left" bit obvious.

Same for `Opr` below


================
Comment at: llvm/lib/Support/FileCheck.cpp:196
 
-  uint64_t Value;
-  switch (Operator) {
-  case '+':
-    Value = LineNumber + Offset;
-    break;
-  case '-':
-    Value = LineNumber - Offset;
-    break;
-  default:
-    SM.PrintMessage(OpLoc, SourceMgr::DK_Error,
-                    Twine("unsupported numeric operation '") + Twine(Operator) +
-                        "'");
-    return nullptr;
-  }
-  return Context->makeNumExpr(Value);
+  return Context->makeNumExpr(new FileCheckASTBinop(EvalBinop, Opl, Opr));
 }
----------------
`make_unique`?


================
Comment at: llvm/lib/Support/FileCheck.cpp:656
 
-template <class... Types>
-FileCheckNumExpr *FileCheckPatternContext::makeNumExpr(Types... Args) {
-  NumExprs.emplace_back(new FileCheckNumExpr(Args...));
+FileCheckNumExpr *FileCheckPatternContext::makeNumExpr(FileCheckASTBinop *AST) {
+  NumExprs.emplace_back(new FileCheckNumExpr(AST));
----------------
I think this should take a `unique_ptr` for AST and use `std::move()` to pass it to the constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60385