[PATCH] D60388: FileCheck [8/12]: Define numeric var from expr

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

thopre added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1878
+          std::move(*ExpressionASTResult);
+      Expected<uint64_t> Value = ExpressionAST->eval();
+      if (!Value) {
----------------
jhenderson wrote:
> jhenderson wrote:
> > Why is the expression being evaluated here? This probably needs a comment.
> "**Now evaluate** the **expression,** whose value this variable should be set **to,** since the expression of a command-line variable definition should only use **variables** defined earlier on the command-line."
I'm confused by what aspect of this you want to be explained. The reworded text seems to explain why is the expression evaluated but I thought you were asking why is it evaluated *here*. If the former, the text should be reworded further to remove the mention about errors and only say that the value of a variable can be given by an expression using variable earlier on the command line. If the latter, then the "only" becomes important since we are saying both that it is safe to evaluate now (we have the value of all variables) and that we should be doing it now (there is no input to match, all variables used are defined either from earlier command-line or from literal values).


================
Comment at: llvm/test/FileCheck/numeric-defines-diagnostics.txt:5
 RUN: not FileCheck -D#10VALUE=10 --input-file %s %s 2>&1 \
 RUN:   | FileCheck %s --strict-whitespace --check-prefix NUMERRCLIFMT
 
----------------
jhenderson wrote:
> By the way, if you were to use --match-full-lines, you wouldn't need to use the regular expression in the last line of each of these checks, and your arrow would point closer to where it should be.
Rather further away since the previous line would still need {{ and escaping for [ while the caret check would not need anything. It does reduce the amount of syntax for that line but it also make other lines slightly more difficult to read since I have to remote the space after the colon due to the --strict-whitespace:

NUMERRCLIFMT:Global defines:1:46: error: invalid variable name

Do you think it's worth doing? I can schedule a separate patch if yes.


================
Comment at: llvm/test/FileCheck/numeric-expression.txt:91
+; CHECK-LABEL: EMPTY NUM EXPR
+; CHECK-NEXT: foo [[#]] bar
+
----------------
jhenderson wrote:
> This is allowed? That's surprising to me, although I suppose it's a natural consequence of the rules.
As you said it's a natural consequence and I thought it could actually be useful (to check you have a number in an error message but don't care about the value) so I didn't forbid it.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:88
+  // clears expression.
+  std::unique_ptr<FileCheckNumericVariableUse> FooVarUseUnique =
+      llvm::make_unique<FileCheckNumericVariableUse>("FOO", &FooVar);
----------------
jhenderson wrote:
> "UseUnique" doesn't seem to line up with what this test is supposed to be doing, from what I can tell. What is "Unique" about it?
There is already a FooVarUse of type FileCheckNumericVariableUse. This one is a unique_ptr of that type. I've renamed into FooVarUsePtr.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:561
+                                      /*LineNumber=*/1, &Cxt, SM);
+  EXPECT_TRUE(bool(ExpressionAST));
+  ExpressionVal = (*ExpressionAST)->eval();
----------------
jhenderson wrote:
> There are probably many other places where this is a problem already, but this should be ASSERT_TRUE, since the test cannot continue if it fails, and will crash on the next line (probably). Perhaps a separate commit to clean those up would be appropriate.
Wasn't aware of ASSERT_TRUE. Another patch to schedule.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60388