Page MenuHomePhabricator

FileCheck [4/12]: Introduce @LINE numeric expressions

Authored by thopre on Apr 7 2019, 4:19 PM.



This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch introduces the @LINE numeric

This commit introduces a new syntax to express a relation a numeric
value in the input text must have with the line number of a given CHECK
pattern: #<@LINE numeric expression>. Further commits build on that
to express relations between several numeric values in the input text.
To help with naming, regular variables are renamed into pattern
variables and old @LINE expression syntax is referred to as legacy
numeric expression.

Compared to existing @LINE expressions, this new syntax allow arbitrary
spacing between the component of the expression. It offers otherwise the
same functionality but the commit serves to introduce some of the data
structure needed to support more general numeric expressions.


  • Linaro (changes up to diff 183612 of revision D55940)
  • GraphCore (changes in later versions of revision D55940 and in new revision created off D55940)

Diff Detail


Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
thopre updated this revision to Diff 197114.Apr 29 2019, 8:15 AM

Rebase on latest changes

thopre updated this revision to Diff 197291.Apr 30 2019, 4:00 AM

Rebase on top of latest master

Not had time to go over the unit tests yet, but I've made lots of other comments.

573 ↗(On Diff #197114)

[[#@LINE]], [[#@LINE+<offset>]], and [[#@LINE-<offset>]]

574 ↗(On Diff #197114)

"with an arbitrary number of spaces between each element of the expression"

575 ↗(On Diff #197114)

a number -> the number

592 ↗(On Diff #197114)

... without any space where -> ..., without any spaces, where

I think you can just delete the word "immediate" from the end of this sentence.

44 ↗(On Diff #197114)

I don't think this is a strict requirement, but I think it would be worth putting everything in a namespace, to allow these classes to all be called NumericExpression, PatternContext etc without any abbreviations (or with fewer). That would make this code a little more descriptive and less repetitive everywhere.

If you did that, you should probably do it in a different change.

46 ↗(On Diff #197114)

Nit: too many full stops.

62 ↗(On Diff #197114)

Subst -> Substitution.

64 ↗(On Diff #197114)

I'd delete the phrase "among other thing" (aside: it should be among other things)

76 ↗(On Diff #197114)

Maybe worth calling this SubstitutionStr or TargetStr or similar. "Substr" isn't necessarily an obvious abbreviation.

80 ↗(On Diff #197114)

= nullptr;

83 ↗(On Diff #197114)

size_t would be more appropriate than unsigned (also in the constructors).

102 ↗(On Diff #197114)

Same comments as above re. abbreviation

111 ↗(On Diff #197114)

Maybe call this function getResult to avoid ambiguity?

113–114 ↗(On Diff #197114)

... if any ... -> ..., if any, ...

173–174 ↗(On Diff #197114)

table, back-references -> table. Back-references

177 ↗(On Diff #197114)

numeric expressions parsed -> parsed numeric expressions

187 ↗(On Diff #197114)

Spurious double space between "command" and "line". Also add a comma after "line".

200 ↗(On Diff #197114)

This should probably include "for destruction" somewhere in its function name, as the name does not imply this at all.

222 ↗(On Diff #197114)

Substs -> Substitutions.

274 ↗(On Diff #197114)


84 ↗(On Diff #197114)

I'm not sure this function gives us anything? Just do it directly in the code. Also, inline here is unnecessary.

If you're bothered by the repeated " \t", then pull that out into a string literal.

235–237 ↗(On Diff #197114)

You're mixing your grammar options here! Either "come in two forms. The first is [[foo:.*]]..." or later "variable 'foo', and [[foo]], which is..."

239–241 ↗(On Diff #197114)

I'd restructure this whole comment slightly from <pattern variable forms><pattern variable names><numeric expression forms><numeric variable names> to <pattern variable forms><numberic expression forms><variable names (for both kinds>.

254 ↗(On Diff #197114)

Use StringRef/Twine here instead of std::string.

259 ↗(On Diff #197114)

It's not really clear to me why the numbers in this expression have been picked. Assuming they represent string lengths, use something like sizeof("string") for the first, and use a ternary for the second (i.e. + IsNumExpr ? sizeof("#") : 0).

282 ↗(On Diff #197114)

size_t here.

319 ↗(On Diff #197114)

As earlier: Subst -> Substitution

408 ↗(On Diff #197114)

Not something you've written, but this should be a size_t too.

473 ↗(On Diff #197114)

Delete "what".

482 ↗(On Diff #197114)

print undefined -> print the undefined.

1 ↗(On Diff #197114)

Nit: double space.

12 ↗(On Diff #197114)

Missed this previously. This should be BAD11 and ERR11, right?

First pass looking at docs and commentary.

574 ↗(On Diff #197291)

with an arbitrary number of spaces

575 ↗(On Diff #197291)

expand to the number of the line where the pattern

592 ↗(On Diff #197291)

with no spaces inside the brackets and where

46 ↗(On Diff #197291)

Extra trailing period.

235 ↗(On Diff #197291)

Hm. The *variable* does not come in two forms; perhaps we should call these "pattern expressions"? Which come in two forms, one that defines a pattern variable and one that uses it.

241 ↗(On Diff #197291)

I thought numeric variables didn't exist until a later patch?

thopre updated this revision to Diff 197353.Apr 30 2019, 9:36 AM
thopre marked 40 inline comments as done.

Address all outstanding review comments except for the namespace one

44 ↗(On Diff #197114)

You mean put all FileCheck classes in a FileCheck namespace?

241 ↗(On Diff #197291)

Regular numeric variables yes but this patch rebrand @LINE as being a pseudo numeric variable and expressions being based on it as being a numeric expression (if of the form #<something>) or legacy numeric expression.

12 ↗(On Diff #197114)

Oops good catch. Had to fix the test as well so I've put it as a separate patch

thopre updated this revision to Diff 197481.Apr 30 2019, 4:08 PM

Rename registerNumExprForDestruction into makeNumExpr

thopre updated this revision to Diff 197485.Apr 30 2019, 4:48 PM

Fix codestyle

jhenderson added inline comments.May 1 2019, 2:11 AM
44 ↗(On Diff #197114)

Yes, that was what I was referring to (or similar to that anyway). Quite possibly this was discussed before.

241 ↗(On Diff #197485)

help -> helps

149 ↗(On Diff #197485)

Substitute -> Substitution

thopre updated this revision to Diff 197520.May 1 2019, 2:55 AM
thopre marked 2 inline comments as done.
  • help -> helps
  • Substitute -> Substitution
thopre updated this revision to Diff 197523.May 1 2019, 3:12 AM

Pattern variables and numeric expressions -> Pattern and numeric expressions

jhenderson accepted this revision.May 1 2019, 5:59 AM

I'm happy with the detail, but Paul may have other comments, especially higher level ones (he has a better eye for them than I do).

235 ↗(On Diff #197523)

expressions -> expression. You don't need to pluralise both that and matches.

This revision is now accepted and ready to land.May 1 2019, 5:59 AM
thopre updated this revision to Diff 197542.May 1 2019, 7:20 AM
  • without no space -> with no space
  • pattern variables and numeric variables -> pattern and numeric variables
thopre updated this revision to Diff 197555.May 1 2019, 8:08 AM
thopre marked an inline comment as done.

expressions matches -> expression matches

Another round of commentary nits, plus a couple more substantive remarks. If you accept them all, LGTM.

529 ↗(On Diff #197353)

supports pattern expressions that allow

566 ↗(On Diff #197353)

which -> that

574 ↗(On Diff #197353)

each element

592 ↗(On Diff #197353)

"without any spaces" or "with no spaces"

529 ↗(On Diff #197555)


44 ↗(On Diff #197114)

+1 for namespace, eventually, although most namespaces are lowercase so I'd prefer 'filecheck'.

108 ↗(On Diff #197353)

or None if substitution failed.

241 ↗(On Diff #197291)

Right, however the comment is describing the naming rule, and right now the naming rule for numeric variables is "@LINE".
I recognize that when we're done, the comment will be right, but for this patch it is not, and the commentary should describe the code as-it-is and not as-we-wish-it-to-be-later.

thopre updated this revision to Diff 197606.May 1 2019, 11:40 AM
thopre marked 7 inline comments as done.

Address Paul's review comments

This revision was automatically updated to reflect the committed changes.