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

[grimar]

Summary

I've noticed that here are
few std::vector<std::string> members in FileCheckRequest,
though there is no reason not to use StringRef it seems.

This patch changes these arrays to std::vector<StringRef>
and refactors the code in callers to cleanup/improve/simplify
how it works with these arrays.

[thopre]

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

llvm/unittests/Support/FileCheckTest.cpp

888–889

Move this below (see comment)

894–896

Fold first line into the second as you did above.

895–898

Ditto and remove the clear

900–902

Ditto

901–904

Ditto

906–908

Ditto

907–910

Ditto

912–915

Ditto

916–919

Ditto

922–923

Ditto

926

Remove and move the declaration of GlobalDefines here.

[grimar]

• Addressed review comments.

[thopre]

LGTM

[MaskRay]

I am a bit concerned that some StringRefs come from the storage of cl::list<string>, and it is not entirely clear that those StringRefs will not get invalidated. (It will not if one knows the internals of cl::list but this is subtle and it'd be good to drop reliance of it.)

Other cleanups, look good, though.

• auto -> concrete type
• emplace_back(std::string(...)) -> emplace_back(...)

[grimar]

In D78202#1984110, @MaskRay wrote:

I am a bit concerned that some StringRefs come from the storage of cl::list<string>, and it is not entirely clear that those StringRefs will not get invalidated. (It will not if one knows the internals of cl::list but this is subtle and it'd be good to drop reliance of it.)

The storage is destroyed when FileCheck terminates for me and with the current code it doesn't seem that any list_storage<DataType, bool> methods that can invalidate it are called/can be called before that.
But I agree that it is too subtle. I'll update the patch to improve this situation.

[grimar]

• Use BumpPtrAllocator to hold a copy of strings.

[MaskRay]

llvm/utils/FileCheck/FileCheck.cpp

568

--check-prefix, --implicit-check-not and -D are by no means performance/memory usage critical... Using a BumpPtrAllocator does not seem to improve the code ?

Can other cleanups in this patch be retained if you don't change the 3 variables to std::vector<StringRef>?

[grimar]

llvm/utils/FileCheck/FileCheck.cpp

568

I think having

std::vector<StringRef> CheckPrefixes;
std::vector<StringRef> ImplicitCheckNot;
std::vector<StringRef> GlobalDefines;

instead of

std::vector<std::string> CheckPrefixes;
std::vector<std::string> ImplicitCheckNot;
std::vector<std::string> GlobalDefines;

Improves the code by itself as readers should not think about why "std::string" was used.
We can revert to the previous revision of the patch as it works fine even without
adding a "BumpPtrAllocator".

[grimar]

llvm/utils/FileCheck/FileCheck.cpp

568

Also, for example this patch changes:

Error FileCheckPatternContext::defineCmdlineVariables(
    std::vector<std::string> &CmdlineDefines, SourceMgr &SM) {

to

Error FileCheckPatternContext::defineCmdlineVariables(ArrayRef<StringRef> CmdlineDefines,
                               SourceMgr &SM);

Using of std::string would require changing CmdlineDefines to be ArrayRef<std::string>,
i.e. std::string is spreading for no reason and the idea of this patch is to stop it.

[grimar]

Thinking about it again I do not think that is *too* subtle to hold StringRef for options like
static cl::list<std::string> ImplicitCheckNot. They are static variables and expected to live the whole
time while application is running. It also would be strange for them to change after initialization,
hence:

• Reverted to the previous version (without BumpPtrAllocator).

llvm/include/llvm/Support/FileCheck.h

 #include "llvm/Support/SourceMgr.h"
 #include <string>
 #include <vector>
 
 namespace llvm {
 
 /// Contains info about various FileCheck options.
 struct FileCheckRequest {
-  std::vector<std::string> CheckPrefixes;
+  std::vector<StringRef> CheckPrefixes;
   bool NoCanonicalizeWhiteSpace = false;
-  std::vector<std::string> ImplicitCheckNot;
-  std::vector<std::string> GlobalDefines;
+  std::vector<StringRef> ImplicitCheckNot;
+  std::vector<StringRef> GlobalDefines;
   bool AllowEmptyInput = false;
   bool MatchFullLines = false;
   bool IgnoreCase = false;
   bool IsDefaultCheckPrefix = false;
   bool EnableVarScope = false;
   bool AllowDeprecatedDagOverlap = false;
   bool Verbose = false;
   bool VerboseVerbose = false;

llvm/lib/Support/FileCheck.cpp

bool FileCheck::readCheckFile(
   if (DefineError) {
     logAllUnhandledErrors(std::move(DefineError), errs());
     return true;
   }
 
   PatternContext->createLineVariable();
 
   std::vector<Pattern> ImplicitNegativeChecks;
-  for (const auto &PatternString : Req.ImplicitCheckNot) {
+  for (StringRef PatternString : Req.ImplicitCheckNot) {
     // Create a buffer with fake command line content in order to display the
     // command line option responsible for the specific implicit CHECK-NOT.
     std::string Prefix = "-implicit-check-not='";
     std::string Suffix = "'";
     std::unique_ptr<MemoryBuffer> CmdLine = MemoryBuffer::getMemBufferCopy(
-        Prefix + PatternString + Suffix, "command line");
+        (Prefix + PatternString + Suffix).str(), "command line");
 
     StringRef PatternInBuffer =
         CmdLine->getBuffer().substr(Prefix.size(), PatternString.size());
     unsigned BufferID = SM.AddNewSourceBuffer(std::move(CmdLine), SMLoc());
     if (ImpPatBufferIDRange) {
       if (ImpPatBufferIDRange->first == ImpPatBufferIDRange->second) {
         ImpPatBufferIDRange->first = BufferID;
         ImpPatBufferIDRange->second = BufferID + 1;
bool FileCheck::readCheckFile(
   }
 
   // When there are no used prefixes we report an error except in the case that
   // no prefix is specified explicitly but -implicit-check-not is specified.
   if (!FoundUsedPrefix &&
       (ImplicitNegativeChecks.empty() || !Req.IsDefaultCheckPrefix)) {
     errs() << "error: no check strings found with prefix"
            << (Req.CheckPrefixes.size() > 1 ? "es " : " ");
-    auto I = Req.CheckPrefixes.begin();
-    auto E = Req.CheckPrefixes.end();
-    if (I != E) {
-      errs() << "\'" << *I << ":'";
-      ++I;
+    for (size_t I = 0, E = Req.CheckPrefixes.size(); I != E; ++I) {
+      if (I != 0)
+        errs() << ", ";
+      errs() << "\'" << Req.CheckPrefixes[I] << ":'";
     }
-    for (; I != E; ++I)
-      errs() << ", \'" << *I << ":'";
-
     errs() << '\n';
     return true;
   }
 
   // Add an EOF pattern for any trailing --implicit-check-not/CHECK-DAG/-NOTs,
   // and use the first prefix as a filler for the error message.
   if (!DagNotMatches.empty()) {
     CheckStrings->emplace_back(
static bool ValidateCheckPrefix(StringRef CheckPrefix) {
   return Validator.match(CheckPrefix);
 }
 
 bool FileCheck::ValidateCheckPrefixes() {
   StringSet<> PrefixSet;
 
   for (StringRef Prefix : Req.CheckPrefixes) {
     // Reject empty prefixes.
-    if (Prefix == "")
+    if (Prefix.empty())
       return false;
 
     if (!PrefixSet.insert(Prefix).second)
       return false;
 
     if (!ValidateCheckPrefix(Prefix))
       return false;
   }
 
   return true;
 }
 
 Regex FileCheck::buildCheckPrefixRegex() {
   // I don't think there's a way to specify an initial value for cl::list,
   // so if nothing was specified, add the default
   if (Req.CheckPrefixes.empty()) {
     Req.CheckPrefixes.push_back("CHECK");
     Req.IsDefaultCheckPrefix = true;
   }
 
   // We already validated the contents of CheckPrefixes so just concatenate
   // them as alternatives.
   SmallString<32> PrefixRegexStr;
-  for (StringRef Prefix : Req.CheckPrefixes) {
-    if (Prefix != Req.CheckPrefixes.front())
+  for (size_t I = 0, E = Req.CheckPrefixes.size(); I != E; ++I) {
+    if (I != 0)
       PrefixRegexStr.push_back('|');
-
-    PrefixRegexStr.append(Prefix);
+    PrefixRegexStr.append(Req.CheckPrefixes[I]);
   }
 
   return Regex(PrefixRegexStr);
 }
 
 Error FileCheckPatternContext::defineCmdlineVariables(
-    std::vector<std::string> &CmdlineDefines, SourceMgr &SM) {
+    ArrayRef<StringRef> CmdlineDefines, SourceMgr &SM) {
   assert(GlobalVariableTable.empty() && GlobalNumericVariableTable.empty() &&
          "Overriding defined variable with command-line variable definitions");
 
   if (CmdlineDefines.empty())
     return Error::success();
 
   // Create a string representing the vector of command-line definitions. Each
   // definition is on its own line and prefixed with a definition number to

llvm/lib/Support/FileCheckImpl.h

public:
   /// \returns the value of string variable \p VarName or an error if no such
   /// variable has been defined.
   Expected<StringRef> getPatternVarValue(StringRef VarName);
 
   /// Defines string and numeric variables from definitions given on the
   /// command line, passed as a vector of [#]VAR=VAL strings in
   /// \p CmdlineDefines. \returns an error list containing diagnostics against
   /// \p SM for all definition parsing failures, if any, or Success otherwise.
-  Error defineCmdlineVariables(std::vector<std::string> &CmdlineDefines,
+  Error defineCmdlineVariables(ArrayRef<StringRef> CmdlineDefines,
                                SourceMgr &SM);
 
   /// Create @LINE pseudo variable. Value is set when pattern are being
   /// matched.
   void createLineVariable();
 
   /// Undefines local variables (variables whose name does not start with a '$'
   /// sign), i.e. removes them from GlobalVariableTable and from

llvm/unittests/Support/FileCheckTest.cpp

private:
   size_t LineNumber = 1;
   SourceMgr SM;
   FileCheckRequest Req;
   FileCheckPatternContext Context;
   Pattern P{Check::CheckPlain, &Context, LineNumber};
 
 public:
   PatternTester() {
-    std::vector<std::string> GlobalDefines;
-    GlobalDefines.emplace_back(std::string("#FOO=42"));
-    GlobalDefines.emplace_back(std::string("BAR=BAZ"));
+    std::vector<StringRef> GlobalDefines = {"#FOO=42", "BAR=BAZ"};
     // An ASSERT_FALSE would make more sense but cannot be used in a
     // constructor.
     EXPECT_THAT_ERROR(Context.defineCmdlineVariables(GlobalDefines, SM),
                       Succeeded());
     Context.createLineVariable();
     // Call parsePattern to have @LINE defined.
     P.parsePattern("N/A", "CHECK", SM, Req);
     // parsePattern does not expect to be called twice for the same line and
TEST_F(FileCheckTest, Match) {
   EXPECT_FALSE(Tester.parsePattern("[[#@LINE]]"));
   EXPECT_THAT_EXPECTED(Tester.match(std::to_string(Tester.getLineNumber())),
                        Succeeded());
 }
 
 TEST_F(FileCheckTest, Substitution) {
   SourceMgr SM;
   FileCheckPatternContext Context;
-  std::vector<std::string> GlobalDefines;
-  GlobalDefines.emplace_back(std::string("FOO=BAR"));
-  EXPECT_THAT_ERROR(Context.defineCmdlineVariables(GlobalDefines, SM),
+  EXPECT_THAT_ERROR(Context.defineCmdlineVariables({"FOO=BAR"}, SM),
                     Succeeded());
 
   // Substitution of an undefined string variable fails and error holds that
   // variable's name.
   StringSubstitution StringSubstitution(&Context, "VAR404", 42);
   Expected<std::string> SubstValue = StringSubstitution.getResult();
   expectUndefErrors({"VAR404"}, SubstValue.takeError());
 
TEST_F(FileCheckTest, Substitution) {
   Pattern P(Check::CheckPlain, &Context, 1);
   StringSubstitution = llvm::StringSubstitution(&Context, "FOO", 42);
   SubstValue = StringSubstitution.getResult();
   ASSERT_THAT_EXPECTED(SubstValue, Succeeded());
   EXPECT_EQ("BAR", *SubstValue);
 }
 
 TEST_F(FileCheckTest, FileCheckContext) {
   FileCheckPatternContext Cxt;
-  std::vector<std::string> GlobalDefines;
   SourceMgr SM;
 
   // No definition.
-  EXPECT_THAT_ERROR(Cxt.defineCmdlineVariables(GlobalDefines, SM), Succeeded());
+  EXPECT_THAT_ERROR(Cxt.defineCmdlineVariables({}, SM), Succeeded());
 
   // Missing equal sign.
-  GlobalDefines.emplace_back(std::string("LocalVar"));
   expectDiagnosticError("missing equal sign in global definition",
-                        Cxt.defineCmdlineVariables(GlobalDefines, SM));
-  GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#LocalNumVar"));
+                        Cxt.defineCmdlineVariables({"LocalVar"}, SM));
   expectDiagnosticError("missing equal sign in global definition",
-                        Cxt.defineCmdlineVariables(GlobalDefines, SM));
+                        Cxt.defineCmdlineVariables({"#LocalNumVar"}, SM));
 
   // Empty variable name.
-  GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("=18"));
   expectDiagnosticError("empty variable name",
-                        Cxt.defineCmdlineVariables(GlobalDefines, SM));
-  GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#=18"));
+                        Cxt.defineCmdlineVariables({"=18"}, SM));
   expectDiagnosticError("empty variable name",
-                        Cxt.defineCmdlineVariables(GlobalDefines, SM));
+                        Cxt.defineCmdlineVariables({"#=18"}, SM));
 
   // Invalid variable name.
-  GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("18LocalVar=18"));
   expectDiagnosticError("invalid variable name",
-                        Cxt.defineCmdlineVariables(GlobalDefines, SM));
-  GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#18LocalNumVar=18"));
+                        Cxt.defineCmdlineVariables({"18LocalVar=18"}, SM));
   expectDiagnosticError("invalid variable name",
-                        Cxt.defineCmdlineVariables(GlobalDefines, SM));
+                        Cxt.defineCmdlineVariables({"#18LocalNumVar=18"}, SM));
 
   // Name conflict between pattern and numeric variable.
-  GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("LocalVar=18"));
-  GlobalDefines.emplace_back(std::string("#LocalVar=36"));
-  expectDiagnosticError("string variable with name 'LocalVar' already exists",
-                        Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  expectDiagnosticError(
+      "string variable with name 'LocalVar' already exists",
+      Cxt.defineCmdlineVariables({"LocalVar=18", "#LocalVar=36"}, SM));
   Cxt = FileCheckPatternContext();
-  GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#LocalNumVar=18"));
-  GlobalDefines.emplace_back(std::string("LocalNumVar=36"));
   expectDiagnosticError(
       "numeric variable with name 'LocalNumVar' already exists",
-      Cxt.defineCmdlineVariables(GlobalDefines, SM));
+      Cxt.defineCmdlineVariables({"#LocalNumVar=18", "LocalNumVar=36"}, SM));
   Cxt = FileCheckPatternContext();
 
   // Invalid numeric value for numeric variable.
-  GlobalDefines.clear();
-  GlobalDefines.emplace_back(std::string("#LocalNumVar=x"));
-  expectUndefErrors({"x"}, Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  expectUndefErrors({"x"}, Cxt.defineCmdlineVariables({"#LocalNumVar=x"}, SM));
 
   // Define local variables from command-line.
-  GlobalDefines.clear();
+  std::vector<StringRef> GlobalDefines;
   // Clear local variables to remove dummy numeric variable x that
   // parseNumericSubstitutionBlock would have created and stored in
   // GlobalNumericVariableTable.
   Cxt.clearLocalVars();
-  GlobalDefines.emplace_back(std::string("LocalVar=FOO"));
-  GlobalDefines.emplace_back(std::string("EmptyVar="));
-  GlobalDefines.emplace_back(std::string("#LocalNumVar1=18"));
-  GlobalDefines.emplace_back(std::string("#%x,LocalNumVar2=LocalNumVar1+2"));
-  GlobalDefines.emplace_back(std::string("#LocalNumVar3=0xc"));
+  GlobalDefines.emplace_back("LocalVar=FOO");
+  GlobalDefines.emplace_back("EmptyVar=");
+  GlobalDefines.emplace_back("#LocalNumVar1=18");
+  GlobalDefines.emplace_back("#%x,LocalNumVar2=LocalNumVar1+2");
+  GlobalDefines.emplace_back("#LocalNumVar3=0xc");
   ASSERT_THAT_ERROR(Cxt.defineCmdlineVariables(GlobalDefines, SM), Succeeded());
 
   // Create @LINE pseudo numeric variable and check it is present by matching
   // it.
   size_t LineNumber = 1;
   Pattern P(Check::CheckPlain, &Cxt, LineNumber);
   FileCheckRequest Req;
   Cxt.createLineVariable();
#endif
   expectUndefErrors({"LocalNumVar2"}, ExpressionVal.takeError());
   EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
   expectUndefErrors({"EmptyVar"}, EmptyVar.takeError());
   // Clear again because parseNumericSubstitutionBlock would have created a
   // dummy variable and stored it in GlobalNumericVariableTable.
   Cxt.clearLocalVars();
 
   // Redefine global variables and check variables are defined again.
-  GlobalDefines.emplace_back(std::string("$GlobalVar=BAR"));
-  GlobalDefines.emplace_back(std::string("#$GlobalNumVar=36"));
+  GlobalDefines.emplace_back("$GlobalVar=BAR");
+  GlobalDefines.emplace_back("#$GlobalNumVar=36");
   ASSERT_THAT_ERROR(Cxt.defineCmdlineVariables(GlobalDefines, SM), Succeeded());
   StringRef GlobalVarStr = "$GlobalVar";
   StringRef GlobalNumVarRef = bufferize(SM, "$GlobalNumVar");
   Expected<StringRef> GlobalVar = Cxt.getPatternVarValue(GlobalVarStr);
   ASSERT_THAT_EXPECTED(GlobalVar, Succeeded());
   EXPECT_EQ(*GlobalVar, "BAR");
   P = Pattern(Check::CheckPlain, &Cxt, ++LineNumber);
   ExpressionPointer = P.parseNumericSubstitutionBlock(

llvm/utils/FileCheck/FileCheck.cpp

if (DumpInput == DumpInputHelp) {
     return 0;
   }
   if (CheckFilename.empty()) {
     errs() << "<check-file> not specified\n";
     return 2;
   }
 
   FileCheckRequest Req;
-  for (auto Prefix : CheckPrefixes)
+  for (StringRef Prefix : CheckPrefixes)
     Req.CheckPrefixes.push_back(Prefix);
 
-  for (auto CheckNot : ImplicitCheckNot)
+  for (StringRef CheckNot : ImplicitCheckNot)
     Req.ImplicitCheckNot.push_back(CheckNot);
 
   bool GlobalDefineError = false;
-  for (auto G : GlobalDefines) {
+  for (StringRef G : GlobalDefines) {
     size_t EqIdx = G.find('=');
     if (EqIdx == std::string::npos) {
       errs() << "Missing equal sign in command-line definition '-D" << G
              << "'\n";
       GlobalDefineError = true;
       continue;
     }
     if (EqIdx == 0) {