[PATCH] D60384: FileCheck [4/12]: Introduce @LINE numeric expressions
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 30 06:03:25 PDT 2019
jhenderson added a comment.
Not had time to go over the unit tests yet, but I've made lots of other comments.
================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:573
+To support this case, FileCheck allows using ``[[#@LINE]]``,
+``[[#@LINE+<offset>]]``, ``[[#@LINE-<offset>]]`` numeric expressions in
+patterns, with arbitrary number of space between each elements of the
----------------
`[[#@LINE]]`, `[[#@LINE+<offset>]]`, **and** `[[#@LINE-<offset>]]`
================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:574
+``[[#@LINE+<offset>]]``, ``[[#@LINE-<offset>]]`` numeric expressions in
+patterns, with arbitrary number of space between each elements of the
+expression. These expressions expand to a number of the line where a pattern
----------------
"with an arbitrary number of spaces between each element of the expression"
================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:575
+patterns, with arbitrary number of space between each elements of the
+expression. These expressions expand to a number of the line where a pattern
+is located (with an optional integer offset).
----------------
a number -> the number
================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:592
+variable syntax: ``[[@LINE]]``, ``[[@LINE+<offset>]]`` and
+``[[@LINE-<offset>]]`` without any space where ``offset`` is an integer
+immediate.
----------------
... without any space where -> ..., without any spaces, where
I think you can just delete the word "immediate" from the end of this sentence.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:44
+/// Class representing a numeric expression.
+class FileCheckNumExpr {
+private:
----------------
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.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:46
+private:
+ /// Value of the numeric expression..
+ uint64_t Value;
----------------
Nit: too many full stops.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:62
+/// Class representing a substitution to perform in the string to match.
+class FileCheckPatternSubst {
+private:
----------------
Subst -> Substitution.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:64
+private:
+ /// Pointer to a class instance holding among other thing the table with the
+ /// values of live pattern variables at the start of any given CHECK line.
----------------
I'd delete the phrase "among other thing" (aside: it should be among other things)
================
Comment at: llvm/include/llvm/Support/FileCheck.h:76
+ /// expression.
+ StringRef SubstStr;
+
----------------
Maybe worth calling this `SubstitutionStr` or `TargetStr` or similar. "Substr" isn't necessarily an obvious abbreviation.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:80
+ /// class representing that numeric expression.
+ FileCheckNumExpr *NumExpr;
+
----------------
`= nullptr;`
================
Comment at: llvm/include/llvm/Support/FileCheck.h:83
+ // Index in RegExStr of where to do the substitution.
+ unsigned InsertIdx;
+
----------------
size_t would be more appropriate than unsigned (also in the constructors).
================
Comment at: llvm/include/llvm/Support/FileCheck.h:102
+ /// Return the string to be substituted.
+ StringRef getSubstString() const { return SubstStr; }
+
----------------
Same comments as above re. abbreviation
================
Comment at: llvm/include/llvm/Support/FileCheck.h:111
+ /// its definition matched.
+ llvm::Optional<std::string> getSubstitute() const;
+
----------------
Maybe call this function `getResult` to avoid ambiguity?
================
Comment at: llvm/include/llvm/Support/FileCheck.h:113-114
+
+ /// Return the name of the undefined variable used in this substitution if
+ /// any or an empty string otherwise.
+ StringRef getUndefVarName() const;
----------------
... if any ... -> ..., if any, ...
================
Comment at: llvm/include/llvm/Support/FileCheck.h:173-174
+ /// pattern variables defined in previous patterns. In a pattern, only the
+ /// last definition for a given variable is recorded in this table,
+ /// back-references are used for uses after any the other definition.
StringMap<StringRef> GlobalVariableTable;
----------------
table, back-references -> table. Back-references
================
Comment at: llvm/include/llvm/Support/FileCheck.h:177
+ /// Vector holding pointers to all numeric expressions parsed. Used to
+ /// automatically free the numeric expressions once they are guaranteed to no
----------------
numeric expressions parsed -> parsed numeric expressions
================
Comment at: llvm/include/llvm/Support/FileCheck.h:187
- /// Define variables from definitions given on the command line passed as a
- /// vector of VAR=VAL strings in \p CmdlineDefines. Report any error to \p SM
- /// and return whether an error occured.
+ /// Define pattern variables from definitions given on the command line
+ /// passed as a vector of VAR=VAL strings in \p CmdlineDefines. Report any
----------------
Spurious double space between "command" and "line". Also add a comma after "line".
================
Comment at: llvm/include/llvm/Support/FileCheck.h:200
+ /// destroyed.
+ template <class... Types> FileCheckNumExpr *registerNumExpr(Types... Args);
};
----------------
This should probably include "for destruction" somewhere in its function name, as the name does not imply this at all.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:222
+ /// is a pattern variable or a numeric expression.
+ std::vector<FileCheckPatternSubst> Substs;
+
----------------
Substs -> Substitutions.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:274
+ /// numeric variables preventing such a successful substitution.
+ void printSubsts(const SourceMgr &SM, StringRef Buffer,
+ SMRange MatchRange = None) const;
----------------
printSubstitutions
================
Comment at: llvm/lib/Support/FileCheck.cpp:84
+// Parsing helper function that strips all leading whitespace from S.
+static inline void skipWhitespace(StringRef &S) { S = S.ltrim(" \t"); }
+
----------------
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.
================
Comment at: llvm/lib/Support/FileCheck.cpp:235-237
+ // come in two forms: [[foo:.*]] which matches .* (or some other regex) and
+ // assigns it to the FileCheck variable 'foo'. The second form is [[foo]]
+ // which is a substitution of foo's value. Variable names themselves must
----------------
You're mixing your grammar options here! Either "come in two forms. The first is [[foo:.*]]..." or later "variable 'foo', and [[foo]], which is..."
================
Comment at: llvm/lib/Support/FileCheck.cpp:239-241
+ // to catch some common errors. Numeric expressions only have the
+ // substitution mode. Numeric variable names have the same restriction as
+ // their pattern variable counterpart.
----------------
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>.
================
Comment at: llvm/lib/Support/FileCheck.cpp:254
+ SMLoc::getFromPointer(PatternStr.data()), SourceMgr::DK_Error,
+ std::string("Invalid ") + RefTypeStr + " reference, no ]] found");
return true;
----------------
Use StringRef/Twine here instead of std::string.
================
Comment at: llvm/lib/Support/FileCheck.cpp:259
MatchStr = MatchStr.substr(0, End);
- PatternStr = PatternStr.substr(End + 4);
+ PatternStr = PatternStr.substr(End + 4 + (int)IsNumExpr);
----------------
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).
================
Comment at: llvm/lib/Support/FileCheck.cpp:282
+ unsigned SubstInsertIdx = RegExStr.size();
+ FileCheckNumExpr *NumExpr;
----------------
size_t here.
================
Comment at: llvm/lib/Support/FileCheck.cpp:319
+ // CHECK pattern or use of a numeric expression.
+ FileCheckPatternSubst Subst =
+ IsNumExpr
----------------
As earlier: Subst -> Substitution
================
Comment at: llvm/lib/Support/FileCheck.cpp:408
unsigned InsertOffset = 0;
+ // Substitute all pattern variables and numeric expressions whose value is
----------------
Not something you've written, but this should be a size_t too.
================
Comment at: llvm/lib/Support/FileCheck.cpp:473
+ SMRange MatchRange) const {
+ // Print info what we know about substitutions. This covers both uses of
+ // pattern variables and numeric subsitutions.
----------------
Delete "what".
================
Comment at: llvm/lib/Support/FileCheck.cpp:482
+
+ // Substitution failed or is not known at match time, print undefined
+ // variable it uses.
----------------
print undefined -> print the undefined.
================
Comment at: llvm/test/FileCheck/line-count.txt:1
; RUN: FileCheck -input-file %s %s
; RUN: not FileCheck -check-prefix BAD1 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR1 %s
----------------
Nit: double space.
================
Comment at: llvm/test/FileCheck/line-count.txt:12
; RUN: not FileCheck -check-prefix BAD10 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR10 %s
; RUN: not FileCheck -check-prefix BAD10 -input-file %s %s 2>&1 | FileCheck -check-prefix ERR10 %s
13
----------------
Missed this previously. This should be BAD11 and ERR11, right?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60384/new/
https://reviews.llvm.org/D60384
More information about the llvm-commits
mailing list