This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch lift the restriction for a
numeric expression to either be a variable definition or a numeric
expression to try to match.

This commit allows a numeric variable to be set to the result of the
evaluation of a numeric expression after it has been matched
successfully. When it happens, the variable is allowed to be used on
the same line since its value is known at match time.

It also makes use of this possibility to reuse the parsing code to
parse a command-line definition by crafting a mirror string of the
-D option with the equal sign replaced by a colon sign, e.g. for option
'-D#NUMVAL=10' it creates the string
'-D#NUMVAL=10 (parsed as #NUMVAL:10)' where the numeric expression
is parsed to define NUMVAL. This result in a few tests needing updating
for the location diagnostics on top of the tests for the new feature.

It also enables empty numeric expression which match any number without
defining a variable. This is done here rather than in commit #5 of the
patch series because it requires to dissociate automatic regex insertion
in RegExStr from variable definition which would make commit #5 even
bigger than it already is.

That's fine. setValue is called after substitution has happened and match was successful. Therefore ExpressionAST ought to evaluate fine. This is just in case code is reworked in the future and this invariant is broken.

It isn't. LLVM Errors and Expecteds HAVE to be checked. If they aren't it will lead to an abort. From the Programmer's Manual:

All Error instances, whether success or failure, must be either checked or moved from (via std::move or a return) before they are destructed. Accidentally discarding an unchecked error will cause a program abort at the point where the unchecked value’s destructor is run, making it easy to identify and fix violations of this rule.

Oh my bad, didn't make the connection with the Expected type. I think an abort would be fine in this case since that error should never occur in any of the executions. By that I mean that code path should be dead (that function will not be called if the expression does not evaluate). If it does, it's a bug somewhere in the code.

Does that make sense? I've added some comments on the expectation for the caller both in the declaration and just on top of the assert since that was missing.

You should check the Expected return value with something other than an assert. It is not sufficient to claim that the caller should call this only when the Expected result must be success. The contract for using Expected is that you will check the result. You could do something like

if (!EvaluatedValue || *EvaluatedValue != NewValue)
llvm_unreachable("New value does not match expression for this variable");

FTR, I haven't done a rigorous check of all the different new code paths to ensure you've got testing for them all, I'm afraid - the change is too large to sensibly do that, although I accept that it probably needs to be this large.

Prefix1 + DefIndex + Prefix2 is used twice, once in each part of the if. If you factor it out into a separate variable, it'll be much easier to read, I think, and you can then get rid of Prefix1 and Prefix2 as separate variables. You can even fold them into the place where DefIndex is created, to avoid additional non-Twine string concatenations:

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.

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).

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.

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.

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.