[PATCH] D78202: [FileCheck] - Refactor the code related to string arrays. NFCI.

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added a comment.

Thanks for doing this. Looks much nicer, especially in the unit tests. A few suggestions to go further in that area.



================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:813
   FileCheckPatternContext Cxt;
-  std::vector<std::string> GlobalDefines;
+  std::vector<StringRef> GlobalDefines;
   SourceMgr SM;
----------------
Move this below (see comment)


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:820-822
+  GlobalDefines.emplace_back("LocalVar");
   expectDiagnosticError("missing equal sign in global definition",
                         Cxt.defineCmdlineVariables(GlobalDefines, SM));
----------------
Fold first line into the second as you did above.


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:823-826
   GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#LocalNumVar"));
+  GlobalDefines.emplace_back("#LocalNumVar");
   expectDiagnosticError("missing equal sign in global definition",
                         Cxt.defineCmdlineVariables(GlobalDefines, SM));
----------------
Ditto and remove the clear


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:829-832
   GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("=18"));
+  GlobalDefines.emplace_back("=18");
   expectDiagnosticError("empty variable name",
                         Cxt.defineCmdlineVariables(GlobalDefines, SM));
----------------
Ditto


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:833-836
   GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#=18"));
+  GlobalDefines.emplace_back("#=18");
   expectDiagnosticError("empty variable name",
                         Cxt.defineCmdlineVariables(GlobalDefines, SM));
----------------
Ditto


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:839-842
   GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("18LocalVar=18"));
+  GlobalDefines.emplace_back("18LocalVar=18");
   expectDiagnosticError("invalid variable name",
                         Cxt.defineCmdlineVariables(GlobalDefines, SM));
----------------
Ditto


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:843-846
   GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#18LocalNumVar=18"));
+  GlobalDefines.emplace_back("#18LocalNumVar=18");
   expectDiagnosticError("invalid variable name",
                         Cxt.defineCmdlineVariables(GlobalDefines, SM));
----------------
Ditto


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:849-853
   GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("LocalVar=18"));
-  GlobalDefines.emplace_back(std::string("#LocalVar=36"));
+  GlobalDefines.emplace_back("LocalVar=18");
+  GlobalDefines.emplace_back("#LocalVar=36");
   expectDiagnosticError("string variable with name 'LocalVar' already exists",
                         Cxt.defineCmdlineVariables(GlobalDefines, SM));
----------------
Ditto


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:855-860
   GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#LocalNumVar=18"));
-  GlobalDefines.emplace_back(std::string("LocalNumVar=36"));
+  GlobalDefines.emplace_back("#LocalNumVar=18");
+  GlobalDefines.emplace_back("LocalNumVar=36");
   expectDiagnosticError(
       "numeric variable with name 'LocalNumVar' already exists",
       Cxt.defineCmdlineVariables(GlobalDefines, SM));
----------------
Ditto


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:864-866
   GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#LocalNumVar=x"));
+  GlobalDefines.emplace_back("#LocalNumVar=x");
   expectUndefErrors({"x"}, Cxt.defineCmdlineVariables(GlobalDefines, SM));
----------------
Ditto


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:869
   // Define local variables from command-line.
   GlobalDefines.clear();
   // Clear local variables to remove dummy numeric variable x that
----------------
Remove and move the declaration of GlobalDefines here.


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

https://reviews.llvm.org/D78202