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

Wed May 8 02:19:49 PDT 2019

jhenderson added inline 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();
----------------
thopre wrote:
> 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));
LLVM has a hand-rolled `make_unique` in STLExtras.h, so you can use that. There are various reasons for that, but one of the main reasons is to avoid potential memory leaks resulting from a subsequent allocation failure. I'm not sure that your code necessarily has an issue here, but it's good to get into the habit of doing things right due to other situations. You can find various discussions on this topic on the Internet, e.g. here: https://stackoverflow.com/questions/37514509/advantages-of-using-stdmake-unique-over-new-operator.

One issue with using emplace_back and a raw pointer is that the vector size increase may cause an allocation failure. The raw pointer then hasn't been captured by the unique_ptr and so is a leak. I don't think the push_back has the same issue though. It gets worse when multiple pointers are in play with the first allocation working but a later one failing.

================
Comment at: llvm/test/FileCheck/numeric-defines-diagnostics.txt:4
+; Invalid variable name: starts with a digit.
+; RUN: not FileCheck -D#10VALUE=10 -input-file %s %s 2>&1 \
+; RUN:   | FileCheck %s --strict-whitespace -check-prefix NUMERRCLIFMT
----------------
You don't actually need the semi-colons in this file. Lit will work just fine without them. You might want to keep them for the comments though to make them clearer.

================
Comment at: llvm/test/FileCheck/numeric-defines.txt:2
+; Test functionality of -D# option: numeric variables are defined to the right
+; value and CHECK directives using it match as expected given the value set.
+
----------------
using it -> using them

================
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 \
----------------
thopre wrote:
> 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?
> 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?
At the moment, tools that use cl::opt directly (i.e. not via tablegen) support both single and double dashes before every option (it might allow more than that too  or have specific limitations for single-letter options etc, but I haven't checked). FileCheck is one such tool. The documentation and help text usually don't make this clear.

> 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.
This discussion on the mailing list was specifically aimed at the LLVM binutils which are intended to be drop-in replacements for GNU equivalents, where using single-dash on long options can lead to ambiguity with grouped short options (e.g. -abc could mean -a -b -c or --abc). I don't think there's a plan to force it wider than this (specifically the consensus appears to be to do it on a tool-by-tool basis), although we might make the decision at some point to normalise it I guess on other tools.

================
Comment at: llvm/test/FileCheck/pattern-defines.txt:2
+; Test functionality of -D option: pattern variables are defined to the right
+; value and CHECK directives using it match as expected given the value set.
+
----------------
using it -> using them

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60385