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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre marked 10 inline comments as done.
thopre added a comment.

In D60385#1493635 <https://reviews.llvm.org/D60385#1493635>, @probinson wrote:

> I still can't tell what the functional difference is, in pattern-defines.txt.  Everything else looks fine.


There is none, pattern-defines + pattern-defines-diagnostics = old defines.txt + some new comments, as per James' comments.



================
Comment at: llvm/lib/Support/FileCheck.cpp:647-648
+                                     uint64_t OperandRight) {
+  NumExprs.emplace_back(
+      new FileCheckNumExpr(EvalBinop, OperandLeft, OperandRight));
   return NumExprs.back().get();
----------------
jhenderson wrote:
> Use `push_back` and `make_unique` here and below.
Being quite inexperienced with C++, would you mind teaching me why push_back is better in this case? Note that I cannot use make_unique because it needs C++14 or later so I've only changed emplace_back to push_back and kept the existing new as follows:

auto NewNumExpr = new FileCheckNumExpr(EvalBinop, OperandLeft, OperandRight);
NumExprs.push_back(std::unique_ptr<FileCheckNumExpr>(NewNumExpr));


================
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 \
----------------
probinson wrote:
> jhenderson wrote:
> > 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.
> Actually there's work in progress (I believe) to stop supporting single-dash long options, and it would be best to stay ahead of the curve there.
I'm confused, the documentation for FileCheck does not mention options having a single and double dash version for each options. AFAICT it's either double dash or single. Did I miss something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60385