In D60386#1504795, @probinson wrote:

There's still some inconsistency in the terminology, as clearly evidenced by the name and description of the IsNumExpr member of FileCheckPatternSubstitution. That class used to be where we'd find variables (now called pattern variables); but it has had numeric expressions crammed into it, except the comment wants to call it a substitution, and so we have "pattern" and "substitution" and "expression" all competing for clarity.

I'm uncertain what to suggest. It may be that "pattern variable" was not a great name, and instead it should be "string variable" to contrast more naturally with "numeric variable." And then substitutions are just substitutions, which can be either a string substitution or a numeric substitution, with the numeric substitution implicitly using a numeric expression. Following this idea, FileCheckPatternSubstitution would be renamed FileCheckSubstitution and the references to "numeric expression" within the class don't change. I don't know that I'd go so far as to have String and Numeric subclasses of FileCheckSubstitution, the abstraction of a Substitution might not really support that. But the naming can surely be improved.

Use gas block instead of text

Address review comments, in particular:

• Address review comments
• Add final point for all comments in numeric-expression.txt

In D61893#1501244, @jhenderson wrote:

I'm not sure I follow what this achieves. https://llvm.org/docs/CommandGuide/FileCheck.html looks okay to me.... I see the build bot failure. Can you point at documentation that indicates what the problem is?

In D60385#1501063, @jhenderson wrote:

Unfortunately, the /new link for me isn't working for this. Could you point out what has changed since your previous version?

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Undo latest submission

• Address Paul's review comments
• Rename numeric expression subsitution for numeric substitution to clearly distinguish numeric expression (the part that gets evaluated) from the whole #<something> that gets substituted (which might include a numeric variable definition)
• Rename ParsePattern into parsePattern since it is modified

• Address Paul's review comments
• Rename numeric expression subsitution for numeric substitution to clearly distinguish numeric expression (the part that gets evaluated) from the whole #<something> that gets substituted (which might include a numeric variable definition).
• Rename ParsePattern into parsePattern since it is modified

Really submit fix for list in FileCheck.rst

Fix list in FileCheck.rst

Reopen to easily review changes to fix FileCheck.rst build failure

In D60385#1499921, @thopre wrote:

In D60385#1499819, @thopre wrote:

In D60385#1499812, @jhenderson wrote:

Okay, LGTM, but make sure @probinson is happy at least.

He wrote "I am pretty happy with this now, maybe give the other people who have been participating a day or so to chime in again" earlier so will commit this now. Thanks everyone who has tirelessly reviewed this patch! It's good to get the first big patch adding support for numeric expression landing.

FYI I've noticed the sphinx error in [1] and am investigated it. I changed laptop recently and sphinx build was not enabled on the new one which is why I missed it. Am doing a build now with sphinx enabled and will send a patch as soon as I can reproduce the issue.

[1] http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/31133/steps/docs-llvm-html/logs/stdio

In D60385#1499819, @thopre wrote:

In D60385#1499812, @jhenderson wrote:

Okay, LGTM, but make sure @probinson is happy at least.

He wrote "I am pretty happy with this now, maybe give the other people who have been participating a day or so to chime in again" earlier so will commit this now. Thanks everyone who has tirelessly reviewed this patch! It's good to get the first big patch adding support for numeric expression landing.

In D60385#1499812, @jhenderson wrote:

Okay, LGTM, but make sure @probinson is happy at least.

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Address outstanding review comments

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Address outstanding comments

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Fix codestyle

Address all outstanding comments

Use 3rd person for all verbs in ReadCheckFile comment

• Document parameters not mentioned in \p when others in the same function are mentioned
• Only use \returns for the return value

Marking this patch as needing review to double check whether comment update in last version is intelligible and grammatically correct english

Fix typo in comment

In D61679#1495260, @probinson wrote:

Aha. The new versions add the word "pattern" to some of the diagnostics, in pattern-defines-diagnostics.txt. That would be the functional change belonging to D60385, which means that test should not pass with an unmodified FileCheck? Please verify.

I am sure this feels like I'm being very picky, but carefully dividing up functional and non-functional reviews makes the reviewer's life much simpler.

Include update to pattern-defines-diagnostics.txt

Fix splitting of NFC part of defines.txt change

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes

Split changes to defines.txt into a separate patch

In D60385#1494841, @probinson wrote:

In D60385#1494062, @thopre wrote:

In D60385#1493635, @probinson wrote:

I still can't tell what the functional difference is, in pattern-defines.txt. Everything else looks fine.

There is none, pattern-defines + pattern-defines-diagnostics = old defines.txt + some new comments, as per James' comments.

If there's no functional difference, it should be separated into its own NFC patch.

• use llvm_make_unique in makeNumExpr and makeNumericVariable
• remove semicolon prefix in tests except for comments
• Use -- for FileCheck long options

Rebase on top of latest changes

Rebase on top of latest changes.

Rebase on top of latest changes

Rebase on top of latest changes

Rebase on top of latest changes