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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1878
+          std::move(*ExpressionASTResult);
+      Expected<uint64_t> Value = ExpressionAST->eval();
+      if (!Value) {
----------------
thopre wrote:
> 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.
The message content is fine as it stands in the current version. I've just suggested some grammar improvements. I think the meaning remains the same with them.


================
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
 
----------------
thopre wrote:
> 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.
I guess it's not worth it. I just find the whole caret checking basically unreadable due to the regex as it stands. It feels quite fragile too.

Since you've got --strict-whitespace on, you could simplify the caret's line a little:
`NUMERRCLIFMT-NEXT: {{^}}                                 ^`
(with appropriate numbers of spaces).

I don't think the trailing $ is important, really, in this case, but could be persuaded either way. In that case though, I'd put that in it's own `{{$}}` pattern. If you are happy with that, then please schedule a patch for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60388