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

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

thopre marked 2 inline comments as done.
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:
> thopre wrote:
> > 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).
> This comment hasn't been addressed.
Sorry, forgot to submit my replies. Done marks automatically get pushed when submitting a new revision.


================
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:
> thopre wrote:
> > 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.
> This has been marked as Done, but I don't see any changes?
Marked it done because I wrote a reply and was expecting an answer about whether you think it is actually worth it. In the event you think it is indeed worth it, I would only mark the answer as done once the work has been completed.

Unfortunately I forgot to submit my replies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60388