[PATCH] D49084: FileCheck: Add support for variable expressions
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 10 03:28:33 PDT 2018
jhenderson added a comment.
First up, I want to say thank you for doing this! This is one of the major missing features from FileCheck over the test harness I am used to using, and leads to fragile testing where people have hard-coded in values in tests that are subject to change.
Do you intend to add hexadecimal support for this? I think it's fine not in this change, but I think it would be a valuable further addition - a lot of binary dumping tools print in hex. I think we should consider whether this impacts the way it's written as well (because presumably we'll need some special syntax to indicate we are capturing a variable as a hex number, not an integer - note that not all hex numbers are prefixed with "0x" when printed by llvm tools).
Also, what about situations like `[[Var1-Var2]]`? Also, what about `[[Var1 - 1]]` (with the spaces - this is more readable to me)?
I don't fully follow the implementation, as I'm not too familiar with FileCheck's code. I do have a number general nits, which I've tried to comment on where I saw them. One repeated one, I wanted to point out was variable naming, where you've often used lower case - see https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly for details.
================
Comment at: docs/CommandGuide/FileCheck.rst:514-518
-To support this case, FileCheck allows using ``[[@LINE]]``,
-``[[@LINE+<offset>]]``, ``[[@LINE-<offset>]]`` expressions in patterns. These
-expressions expand to a number of the line where a pattern is located (with an
-optional integer offset).
+To support this case, FileCheck understands the ``@LINE`` pseudo variable in
+patterns in both regular variable uses ``[[@LINE]]`` and in expressions
+``[[@LINE+<offset>]]``, ``[[@LINE-<offset>]]``. These expand to a number of
+the line where a pattern is located (with an optional integer offset).
----------------
I'm wondering why this comment has changed?
================
Comment at: test/FileCheck/var-expression.txt:3
+
+; expression using variables defined on other lines
+START
----------------
Not sure if the LLVM coding style applies for test comments, but assuming they do, these need capital letters and trailing full stops.
================
Comment at: test/FileCheck/var-expression.txt:29
+CHECK-LABEL: MARK1
+CHECK-NEXT: [[VAREL:[0-9]+]] [[VAREL+1]]
+CHECK-NEXT: [[VAREL]] [[VAREL-1]]
----------------
VAREL? I have no idea how to expand that name...!
================
Comment at: utils/FileCheck/FileCheck.cpp:112
class Pattern {
+ /// Valid operations in expressions
+ static constexpr StringRef validArithOps = StringRef("+-", 2);
----------------
Missing trailing full stop.
================
Comment at: utils/FileCheck/FileCheck.cpp:113
+ /// Valid operations in expressions
+ static constexpr StringRef validArithOps = StringRef("+-", 2);
+
----------------
Shouldn't this be a capitalized name?
================
Comment at: utils/FileCheck/FileCheck.cpp:422
+/// patterns in earlier lines while LocalVariableTable holds the values of
+/// all definitions of variables defined in the same line. CheckOnly allows
+/// to only check that the expression is well formed, in which case
----------------
Spurious space before CheckOnly.
I might be being a bit dim, but why can't "[[Foo]]" be an expression, if it is missing any arithmetic?
================
Comment at: utils/FileCheck/FileCheck.cpp:428
+/// user-defined variable or LINE builtin variable. For the latter case the
+/// operation is optional, ie @LINE is a valid expression.
+///
----------------
ie -> i.e.
================
Comment at: utils/FileCheck/FileCheck.cpp:435
+ std::string &Value, bool CheckOnly) const {
+ int val;
+ StringRef VarName;
----------------
val -> Val
I'd actually avoid using both val and Value in the same scope, as it gets very confusing. Please come up with a different name for one of them. E.g. Value could become ValueString.
================
Comment at: utils/FileCheck/FileCheck.cpp:470-472
+ char op = Expr[0];
+ Expr = Expr.substr(1);
+ int offset = 0;
----------------
op -> Op
offset -> Offset
================
Comment at: utils/FileCheck/FileCheck.cpp:490
+
+/// Evaluates use of variable Name at \p index in the pattern and stores the
+/// result to \p Value. GlobalVariableTable holds the value of variables
----------------
variable Name -> variable \p Name (I think?)
================
Comment at: utils/FileCheck/FileCheck.cpp:497
+bool Pattern::GetVariableValue(
+ StringRef Name, unsigned index, StringMap<StringRef> &GlobalVariableTable,
+ StringMap<std::vector<std::pair<StringRef, unsigned>>> &LocalVariableTable,
----------------
index -> Index
================
Comment at: utils/FileCheck/FileCheck.cpp:500
+ StringRef &Value) const {
+ bool isExpression = (Name.find_first_of(validArithOps) != StringRef::npos);
+
----------------
isExpression -> IsExpression
================
Comment at: utils/FileCheck/FileCheck.cpp:502-505
+ // Expressions cannot use backreference when referencing variable defined
+ // earlier on the same line due to the resulting value being different than
+ // the one defined. Therefore we need to explicitely check here whether the
+ // variable was defined on the same line at an earlier index for expressions.
----------------
same line due -> same line, due
Remove extra space before "Therefore"
explicitely -> explicitly
================
Comment at: utils/FileCheck/FileCheck.cpp:509
+ auto VariableDefIterator = VariableDefs.find(Name);
+ // At least one definition of this variable exist on the same line at an
+ // earlier index. Search for its value.
----------------
exist -> exists
================
Comment at: utils/FileCheck/FileCheck.cpp:531-532
+ // No local definition found or this is a non-expression variable use, in
+ // which case it refers to a definition on another line since same-line use
+ // are handled by back-references. Search for its earlier definition.
+ StringMap<StringRef>::iterator git = GlobalVariableTable.find(Name);
----------------
same-line use are -> either "same-line uses are" or "same-line use is"
================
Comment at: utils/FileCheck/FileCheck.cpp:533
+ // are handled by back-references. Search for its earlier definition.
+ StringMap<StringRef>::iterator git = GlobalVariableTable.find(Name);
+ if (git == GlobalVariableTable.end())
----------------
git? Please use a more descriptive name.
Also, should be capitalised.
================
Comment at: utils/FileCheck/FileCheck.cpp:569-570
std::string TmpStr;
- if (!VariableUses.empty()) {
- TmpStr = RegExStr;
-
- unsigned InsertOffset = 0;
- for (const auto &VariableUse : VariableUses) {
- std::string Value;
+ StringMap<std::vector<std::pair<StringRef, unsigned>>> LocalVariableTable =
+ {};
+ SmallVector<StringRef, 4> MatchInfo;
----------------
I don't think you need the initialiser here.
================
Comment at: utils/FileCheck/FileCheck.cpp:575-581
+ // Do two passes over the pattern to handle expressions using variable
+ // defined earlier on the same line. First pass will replace all other uses
+ // of variable by their value and then do temporary records of the value of
+ // variables defined in the pattern. Second pass replaces all uses, using
+ // that temporary record. Note that non-expression variable use of variables
+ // defined on the same line are handled via back-reference and thus are not
+ // replaced by any of those passes.
----------------
Are we referring to a specific variable here? If so, please use "the variable" not just "variable". If not, please use "a variable". If there are multiple variables the first sentences refer to, please use the plural to make it clear.
"First pass" -> "The first pass". Similarly "Second pass" -> "The second pass".
I'm a little confused by the first use of variable in this bit: "Note that non-expression variable use of variables...". Maybe it needs re-wording.
================
Comment at: utils/FileCheck/FileCheck.cpp:590-591
+ StringRef Expr = VariableUse.first;
+ bool isPseudo = (Expr[0] == '@');
+ bool isExpression =
+ (Expr.find_first_of(validArithOps) != StringRef::npos);
----------------
is... -> Is...
================
Comment at: utils/FileCheck/FileCheck.cpp:604
+
+ // Wait second pass to replace uses in expression
+ if (State >= DefProcessed || !isExpression)
----------------
I'm not sure how to parse this sentence. I think you might mean: "Wait for the second pass to replace uses in the expression." But I'm not certain.
================
Comment at: utils/FileCheck/FileCheck.cpp:622
+ } else {
+ // During first pass, allow any number of digits instead of the
+ // expressions. This allows more matches in this pass than the
----------------
During first pass -> During the first pass
================
Comment at: utils/FileCheck/FileCheck.cpp:625
+ // actual pattern which might lead to the wrong temporary record
+ // of variable definition at the end of the pass. This in turns
+ // might resolve expression incorrectly in the second pass and
----------------
in turns -> in turn
================
Comment at: utils/FileCheck/FileCheck.cpp:626
+ // of variable definition at the end of the pass. This in turns
+ // might resolve expression incorrectly in the second pass and
+ // give incorrect match, hence the documented limitations on use
----------------
expression -> the expression.
================
Comment at: utils/FileCheck/FileCheck.cpp:655
+ // If this defines any variables, temporarily record their values for each
+ // of the definition to resolve expression in the next pass.
+ if (State == Unprocessed) {
----------------
I'd get rid of "for each of the definition" in this comment, as I think it's implicit. If you think it's important get rid of the "of the".
Also "expression" -> "the expression" or "expressions"
================
Comment at: utils/FileCheck/FileCheck.cpp:679
+ default:
+ assert(0);
+ break;
----------------
Use llvm_unreachable here.
Repository:
rL LLVM
https://reviews.llvm.org/D49084
More information about the llvm-commits
mailing list