[PATCH] D60382: FileCheck [2/12]: Stricter parsing of -D option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:27
 
+bool FileCheckPattern::isValidVarNameStart(char c) {
+  return c == '_' || isalpha(c);
----------------
Here and elsewhere below, I don't believe the style has changed yet and this should be upper-case 'C' (also 'I', 'E' etc below).


================
Comment at: llvm/lib/Support/FileCheck.cpp:171
+      if (parseVariable(MatchStr, IsPseudo, TrailIdx)) {
+        SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data()),
+                        SourceMgr::DK_Error, "Invalid name in named regex");
----------------
Does this print the actual specified string in the error message?


================
Comment at: llvm/lib/Support/FileCheck.cpp:178
+      StringRef Trailer = MatchStr.substr(TrailIdx);
+      size_t DefSepIdx = Trailer.find(":");
+      bool IsVarDef = (DefSepIdx != StringRef::npos);
----------------
DefSepIdx doesn't read well to me (as in I'm not sure what is meant by it at first glance). Perhaps a slightly more verbose name would be clearer.


================
Comment at: llvm/lib/Support/FileCheck.cpp:184
+                        SourceMgr::DK_Error,
+                        "Invalid name in named regex definition");
         return true;
----------------
Lower-case 'i' I think is more in keeping with elsewhere, so I don't think you should change this.


================
Comment at: llvm/lib/Support/FileCheck.cpp:190
       // supports @LINE, @LINE+number, @LINE-number expressions. The check here
       // is relaxed, more strict check is performed in \c EvaluateExpression.
+      if (IsPseudo) {
----------------
Nit: is relaxed -> is a relaxed


================
Comment at: llvm/lib/Support/FileCheck.cpp:1407
+  bool ErrorFound = false;
+  for (const auto &CmdlineDef : CmdlineDefines) {
+    std::string Prefix = "-D";
----------------
Keep the `StringRef` here.


================
Comment at: llvm/lib/Support/FileCheck.cpp:1408
+  for (const auto &CmdlineDef : CmdlineDefines) {
+    std::string Prefix = "-D";
+    std::string DiagCmdline = Prefix + CmdlineDef;
----------------
Use a StringRef here.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:55-63
+  EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('a'));
+  EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('G'));
+  EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('_'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('2'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('$'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('@'));
+  EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('+'));
----------------
These should probably be a separate test since they test a different function.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:70
+  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(TrailIdx, 9U);
+
----------------
Here and below, it might be worthwhile using VarName.size() (with appropriate - 1 as required) to make the test more robust, in case the input name changes ever.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60382