[PATCH] D98691: [FileCheck] Fix PR49531: invalid use of string var

[James Henderson via Phabricator via llvm-commits]

jhenderson 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};
----------------
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?


================
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
----------------
I would suggest you also want an invalid variable name which is a definition (e.g. `[[X-Y:.+]]`) or something like that.


================
Comment at: llvm/unittests/FileCheck/FileCheckTest.cpp:1345
+  // Invalid use of string variable.
+  EXPECT_TRUE(Tester.parsePattern("[[FOO-BAR]]"));
+
----------------
Ditto - consider also checking for a definition with an invalid name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98691/new/

https://reviews.llvm.org/D98691