[PATCH] D98691: [FileCheck] Fix PR49531: invalid use of string var
Thomas Preud'homme via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 17 03:17:45 PDT 2021
thopre added inline comments.
================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:460
StringRef Name = Str.take_front(I);
Str = Str.substr(I);
return VariableProperties {Name, IsPseudo};
----------------
jhenderson wrote:
> I wonder if it would make more sense to add the check to this function, somewhere around here. This is the "parseVariable" function after all, so seems like the logical place to reject invalid variable names, rather than the place it is currently detected. What do you think?
A trailer has to be allowed when parsing a variable use because it could be `[[@LINE+1]]`. If the variable is a pseudo variable, parsing is deferred to the numeric substitution block parsing. Otherwise any trailer is an error. Perhaps it's time to remove all uses of `[[@LINE]]` in favor of `[[#@LINE]]`
================
Comment at: llvm/test/FileCheck/simple-var-capture.txt:22
+; INVALID-VARNAME-NEXT: [[Y:]]
+; INVALID-VARNAME-NEXT: [[X-Y]]
+; INVALID-VARNAME-MSG: simple-var-capture.txt:[[#@LINE-1]]:27: error: invalid name in string variable use
----------------
jhenderson wrote:
> I would suggest you also want an invalid variable name which is a definition (e.g. `[[X-Y:.+]]`) or something like that.
The new code is only executed for string variable use. String variable definition and pseudo variable use different codepath.
================
Comment at: llvm/unittests/FileCheck/FileCheckTest.cpp:1345
+ // Invalid use of string variable.
+ EXPECT_TRUE(Tester.parsePattern("[[FOO-BAR]]"));
+
----------------
jhenderson wrote:
> Ditto - consider also checking for a definition with an invalid name.
See line 1339 for a test for that already.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98691/new/
https://reviews.llvm.org/D98691
More information about the llvm-commits
mailing list