Page MenuHomePhabricator

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

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

Details

Summary

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

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.

Copyright:

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

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.

llvm/docs/CommandGuide/FileCheck.rst
573

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

574

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

575

a number -> the number

592

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

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

llvm/include/llvm/Support/FileCheck.h
44

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

Nit: too many full stops.

62

Subst -> Substitution.

64

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

76

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

80

= nullptr;

83

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

102

Same comments as above re. abbreviation

111

Maybe call this function getResult to avoid ambiguity?

113–114

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

173–174

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

177

numeric expressions parsed -> parsed numeric expressions

187–189

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

200

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

222

Substs -> Substitutions.

274

printSubstitutions

llvm/lib/Support/FileCheck.cpp
84

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

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

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

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

259

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

size_t here.

319

As earlier: Subst -> Substitution

408–409

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

473

Delete "what".

482

print undefined -> print the undefined.

llvm/test/FileCheck/line-count.txt
1

Nit: double space.

12

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

First pass looking at docs and commentary.

llvm/docs/CommandGuide/FileCheck.rst
574

with an arbitrary number of spaces

575

expand to the number of the line where the pattern

592

with no spaces inside the brackets and where

llvm/include/llvm/Support/FileCheck.h
46

Extra trailing period.

llvm/lib/Support/FileCheck.cpp
235

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

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

llvm/include/llvm/Support/FileCheck.h
44

You mean put all FileCheck classes in a FileCheck namespace?

llvm/lib/Support/FileCheck.cpp
241

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.

llvm/test/FileCheck/line-count.txt
12

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
llvm/include/llvm/Support/FileCheck.h
44

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

llvm/lib/Support/FileCheck.cpp
240

help -> helps

llvm/unittests/Support/FileCheckTest.cpp
149

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

llvm/lib/Support/FileCheck.cpp
234–241

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.

llvm/docs/CommandGuide/FileCheck.rst
529–530

supports pattern expressions that allow

529–530

allow

566

which -> that

574

each element

592

"without any spaces" or "with no spaces"

llvm/include/llvm/Support/FileCheck.h
44

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

108

or None if substitution failed.

llvm/lib/Support/FileCheck.cpp
241

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.