Page MenuHomePhabricator

FileCheck: Improve FileCheck variable terminology
ClosedPublic

Authored by thopre on May 20 2019, 8:18 AM.

Details

Summary

Terminology introduced by # blocks is confusing and does not
integrate well with existing terminology.

First, variables referred by [[]] blocks are called "pattern variables"
while the text a CHECK directive needs to match is called a "CHECK
pattern". This is inconsistent with variables in # blocks since
# blocks are also found in CHECK pattern yet those variables are
called "numeric variable".

Second, the replacing of both [[]] and # blocks by the value of the
variable or expression they contain is represented by a
FileCheckPatternSubstitution class. The naming refers to being a
substitution in a CHECK pattern but could be wrongly understood as being
a substitution of a pattern variable.

Third and lastly, comments use "numeric expression" to refer both to the
# blocks as well as to the numeric expressions these blocks contain
which get evaluated at match time.

This patch solves these confusions by

  • calling variables in [[]] and # blocks as string and numeric variables respectively;
  • referring to [[]] and # as substitution *blocks*, with the former being a string substitution block and the latter a numeric substitution block;
  • calling [[]] and # blocks to be replaced by the value of a variable or expression they contain a substitution (as opposed to definition when these blocks are used to defined a variable), with the former being a string substitution and the latter a numeric substitution;
  • renaming the FileCheckPatternSubstitution as a FileCheckSubstitution class with FileCheckStringSubstitution and FileCheckNumericSubstitution subclasses;
  • restricting the use of "numeric expression" to refer to the expression that is evaluated in a numeric substitution.

While numeric substitution blocks only support numeric substitutions of
numeric expressions at the moment there are plans to augment numeric
substitution blocks to support numeric definitions as well as both a
numeric definition and numeric substitution in the same numeric
substitution block.

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.May 20 2019, 8:18 AM
thopre marked an inline comment as done.May 21 2019, 2:08 AM
thopre added inline comments.
llvm/include/llvm/Support/FileCheck.h
314 ↗(On Diff #200333)

Self note: this needs to become a vector of std::unique_ptr

thopre updated this revision to Diff 200465.May 21 2019, 5:30 AM

Add memory management for substitutions

There's quite a bit more functional changes here than I was expecting. I was expecting this to be a pure renaming exercise.

llvm/docs/CommandGuide/FileCheck.rst
503 ↗(On Diff #200465)

This line needs shortening, or the docs will probably fail to build.

594 ↗(On Diff #200465)

which substitution -> which substitute

llvm/include/llvm/Support/FileCheck.h
106 ↗(On Diff #200465)

Does FileCheckPatternContext still make sense as a name?

108–109 ↗(On Diff #200465)

I don't think you need "to match" here, if you are naming the variable directly.

111 ↗(On Diff #200465)

This isn't just a naming change here...!

114 ↗(On Diff #200465)

for the text -> with the text
defined to -> defined as

115 ↗(On Diff #200465)

expression -> expressions

116 ↗(On Diff #200465)

access directly -> directly access

238 ↗(On Diff #200465)

Does FileCheckPattern still make sense in the new naming scheme?

llvm/lib/Support/FileCheck.cpp
314 ↗(On Diff #200465)

subtituion -> substitution

thopre updated this revision to Diff 200690.May 22 2019, 3:45 AM
thopre marked 9 inline comments as done.
  • Split new substitution hierarchy into a separate patch
  • address review comments
thopre marked 3 inline comments as done.May 22 2019, 3:47 AM
thopre added inline comments.
llvm/include/llvm/Support/FileCheck.h
106 ↗(On Diff #200465)

Yes, it refers to a FileCheckPattern which is the part after the colon in a CHECK (or similar) directive. String variables were called pattern variables because patterns only supported one type of variables. Now we have patterns supporting string and numeric variable. The pattern context holds the current live string and numeric variables.

238 ↗(On Diff #200465)

I think so, a pattern refers to what a CHECK directive need to match. In the end it becomes a regular expression pattern that is being matched, whether it includes no variable, string variables, numeric expressions or both.

jhenderson accepted this revision.May 22 2019, 6:01 AM

A few more phrasing issues, otherwise LGTM.

llvm/include/llvm/Support/FileCheck.h
262 ↗(On Diff #200690)

Is the i.e. referring to just the two Global* variables, or is the "and ..." included?

Also:
variable -> variables
clear -> clears
I'd probably change:
"for numeric variable clear their value as well" to "also clears the value of numeric variables".

llvm/lib/Support/FileCheck.cpp
387 ↗(On Diff #200690)

pattern or substitution -> patterns or subsitutions

470 ↗(On Diff #200690)

only known now -> only now known (this sounds clearer to me)

llvm/unittests/Support/FileCheckTest.cpp
231 ↗(On Diff #200690)

of undefined -> of an undefined

This revision is now accepted and ready to land.May 22 2019, 6:01 AM
thopre updated this revision to Diff 200726.May 22 2019, 6:40 AM
thopre marked 5 inline comments as done.

Address review comments

A few small things, and LGTM too.

llvm/docs/CommandGuide/FileCheck.rst
547 ↗(On Diff #200726)

blocks

llvm/include/llvm/Support/FileCheck.h
371 ↗(On Diff #200726)

pattern -> string
remove "such"

llvm/lib/Support/FileCheck.cpp
387 ↗(On Diff #200726)

comma after "patterns"

469 ↗(On Diff #200726)

value is -> values are

thopre updated this revision to Diff 200840.May 22 2019, 5:04 PM
thopre marked 4 inline comments as done.

Address review comments

This revision was automatically updated to reflect the committed changes.