[TableGen] Clang changes to support Record::getValueAsString and getValueAsListOfStrings returning StringRef instead of std::string

[craig.topper]

Summary

This is the clang version of D33710.

[craig.topper]

utils/TableGen/ClangOptionDocEmitter.cpp

272 ↗

(On Diff #100826)

Had to make a copy into vector std::string because emitOptionWithArgs has another caller that passes a std::string vector.

[dblaikie]

Few questions, but address them as you see fit.

utils/TableGen/ClangDiagnosticsEmitter.cpp

1280–1281 ↗

(On Diff #100826)

Any reason (I'm guessing there is) not to do the str() on the + expression instead of the RHS? (seemed like you'd done that in other changes here, to minimize the string buffering/reallocation/etc by stringifying once at the top level of the expression)

utils/TableGen/ClangOptionDocEmitter.cpp

86 ↗

(On Diff #100826)

Does this need to be a std::string here? I'm not spotting any mutation (but I could well be missing it) of the value. Is it that StringREf doesn't support some of these substr - like operations?

Oh, I guess it's that all of these operations want a std::string (for lookup in OptionsByName, for the result of string concatenation, etc).

It'd still be marginally more efficient to not have to create a std::string up-front, I'd think, but syntactically annoying/verbose for all those uses?

272 ↗

(On Diff #100826)

Would it be better/OK if the other caller was the one making the copy (it'd be cheaper to make a std::vector<StringRef> from a std::vector<std::string> than the other way around - but maybe the other caller is much hotter than this one?)?

[MatzeB]

I assume you have checked that the tablegen output doesn't change. Apart from that I'm all for more StringRef and SmallStrings or even Twines where possible. So LGTM.

cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp

public:
 
     void writeDump(raw_ostream &OS) const override {
       OS << "    for (const auto &Val : SA->" << RangeName << "())\n";
       OS << "      OS << \" \" << Val;\n";
     }
   };
 
   // Unique the enums, but maintain the original declaration ordering.
-  std::vector<std::string>
-  uniqueEnumsInOrder(const std::vector<std::string> &enums) {
-    std::vector<std::string> uniques;
+  std::vector<StringRef>
+  uniqueEnumsInOrder(const std::vector<StringRef> &enums) {
+    std::vector<StringRef> uniques;
     SmallDenseSet<StringRef, 8> unique_set;
     for (const auto &i : enums) {
       if (unique_set.insert(i).second)
         uniques.push_back(i);
     }
     return uniques;
   }
 
   class EnumArgument : public Argument {
     std::string type;
-    std::vector<std::string> values, enums, uniques;
+    std::vector<StringRef> values, enums, uniques;
+
   public:
     EnumArgument(const Record &Arg, StringRef Attr)
       : Argument(Arg, Attr), type(Arg.getValueAsString("Type")),
         values(Arg.getValueAsListOfStrings("Values")),
         enums(Arg.getValueAsListOfStrings("Enums")),
         uniques(uniqueEnumsInOrder(enums))
     {
       // FIXME: Emit a proper error
void writeConversion(raw_ostream &OS) const {
       OS << "    }\n"
          << "    llvm_unreachable(\"No enumerator with that value\");\n"
          << "  }\n";
     }
   };
   
   class VariadicEnumArgument: public VariadicArgument {
     std::string type, QualifiedTypeName;
-    std::vector<std::string> values, enums, uniques;
+    std::vector<StringRef> values, enums, uniques;
 
   protected:
     void writeValueImpl(raw_ostream &OS) const override {
       // FIXME: this isn't 100% correct -- some enum arguments require printing
       // as a string literal, while others require printing as an identifier.
       // Tablegen currently does not distinguish between the two forms.
       OS << "    OS << \"\\\"\" << " << getAttrName() << "Attr::Convert" << type
          << "ToStr(Val)" << "<< \"\\\"\";\n";
if (isSubRule()) {
       if (isNegatedSubRule())
         Result += ')';
       Result += ')';
     }
     return Result;
   }
 
   std::string getEnumValueName() const {
-    std::string Result =
-        "SubjectMatchRule_" + MetaSubject->getValueAsString("Name");
+    SmallString<128> Result;
+    Result += "SubjectMatchRule_";
+    Result += MetaSubject->getValueAsString("Name");
     if (isSubRule()) {
       Result += "_";
       if (isNegatedSubRule())
         Result += "not_";
       Result += Constraint->getValueAsString("Name");
     }
     if (isAbstractRule())
       Result += "_abstract";
-    return Result;
+    return Result.str();
   }
 
   std::string getEnumValue() const { return "attr::" + getEnumValueName(); }
 
   static const char *EnumName;
 };
 
 const char *AttributeSubjectMatchRule::EnumName = "attr::SubjectMatchRule";

 }
 
 // Generate a conditional expression to check if the current target satisfies
 // the conditions for a TargetSpecificAttr record, and append the code for
 // those checks to the Test string. If the FnName string pointer is non-null,
 // append a unique suffix to distinguish this set of target checks from other
 // TargetSpecificAttr records.
 static void GenerateTargetSpecificAttrChecks(const Record *R,
-                                             std::vector<std::string> &Arches,
+                                             std::vector<StringRef> &Arches,
                                              std::string &Test,
                                              std::string *FnName) {
   // It is assumed that there will be an llvm::Triple object
   // named "T" and a TargetInfo object named "Target" within
   // scope that can be used to determine whether the attribute exists in
   // a given target.
   Test += "(";
 
   for (auto I = Arches.begin(), E = Arches.end(); I != E; ++I) {
-    std::string Part = *I;
-    Test += "T.getArch() == llvm::Triple::" + Part;
+    StringRef Part = *I;
+    Test += "T.getArch() == llvm::Triple::";
+    Test += Part;
     if (I + 1 != E)
       Test += " || ";
     if (FnName)
       *FnName += Part;
   }
   Test += ")";
 
   // If the attribute is specific to particular OSes, check those.
   if (!R->isValueUnset("OSes")) {
     // We know that there was at least one arch test, so we need to and in the
     // OS tests.
     Test += " && (";
-    std::vector<std::string> OSes = R->getValueAsListOfStrings("OSes");
+    std::vector<StringRef> OSes = R->getValueAsListOfStrings("OSes");
     for (auto I = OSes.begin(), E = OSes.end(); I != E; ++I) {
-      std::string Part = *I;
+      StringRef Part = *I;
 
-      Test += "T.getOS() == llvm::Triple::" + Part;
+      Test += "T.getOS() == llvm::Triple::";
+      Test += Part;
       if (I + 1 != E)
         Test += " || ";
       if (FnName)
         *FnName += Part;
     }
     Test += ")";
   }
 
   // If one or more CXX ABIs are specified, check those as well.
   if (!R->isValueUnset("CXXABIs")) {
     Test += " && (";
-    std::vector<std::string> CXXABIs = R->getValueAsListOfStrings("CXXABIs");
+    std::vector<StringRef> CXXABIs = R->getValueAsListOfStrings("CXXABIs");
     for (auto I = CXXABIs.begin(), E = CXXABIs.end(); I != E; ++I) {
-      std::string Part = *I;
-      Test += "Target.getCXXABI().getKind() == TargetCXXABI::" + Part;
+      StringRef Part = *I;
+      Test += "Target.getCXXABI().getKind() == TargetCXXABI::";
+      Test += Part;
       if (I + 1 != E)
         Test += " || ";
       if (FnName)
         *FnName += Part;
     }
     Test += ")";
   }
 }
if (Variety == "CXX11") {
             break;
           }
       }
     }
 
     std::string Test;
     if (Attr->isSubClassOf("TargetSpecificAttr")) {
       const Record *R = Attr->getValueAsDef("Target");
-      std::vector<std::string> Arches = R->getValueAsListOfStrings("Arches");
+      std::vector<StringRef> Arches = R->getValueAsListOfStrings("Arches");
       GenerateTargetSpecificAttrChecks(R, Arches, Test, nullptr);
 
       // If this is the C++11 variety, also add in the LangOpts test.
       if (Variety == "CXX11")
         Test += " && LangOpts.CPlusPlus11";
     } else if (Variety == "CXX11")
       // C++11 mode should be checked against LangOpts, which is presumed to be
       // present in the caller.
static std::string GenerateTargetRequirements(const Record &Attr,
                                               raw_ostream &OS) {
   // If the attribute is not a target specific attribute, return the default
   // target handler.
   if (!Attr.isSubClassOf("TargetSpecificAttr"))
     return "defaultTargetRequirements";
 
   // Get the list of architectures to be tested for.
   const Record *R = Attr.getValueAsDef("Target");
-  std::vector<std::string> Arches = R->getValueAsListOfStrings("Arches");
+  std::vector<StringRef> Arches = R->getValueAsListOfStrings("Arches");
   if (Arches.empty()) {
     PrintError(Attr.getLoc(), "Empty list of target architectures for a "
                               "target-specific attr");
     return "defaultTargetRequirements";
   }
 
   // If there are other attributes which share the same parsed attribute kind,
   // such as target-specific attributes with a shared spelling, collapse the
   // duplicate architectures. This is required because a shared target-specific
   // attribute has only one AttributeList::Kind enumeration value, but it
   // applies to multiple target architectures. In order for the attribute to be
   // considered valid, all of its architectures need to be included.
   if (!Attr.isValueUnset("ParseKind")) {
     std::string APK = Attr.getValueAsString("ParseKind");
     for (const auto &I : Dupes) {
       if (I.first == APK) {
-        std::vector<std::string> DA = I.second->getValueAsDef("Target")
-                                          ->getValueAsListOfStrings("Arches");
-        std::move(DA.begin(), DA.end(), std::back_inserter(Arches));
+        std::vector<StringRef> DA =
+            I.second->getValueAsDef("Target")->getValueAsListOfStrings(
+                "Arches");
+        Arches.insert(Arches.end(), DA.begin(), DA.end());
       }
     }
   }
 
   std::string FnName = "isTarget";
   std::string Test;
   GenerateTargetSpecificAttrChecks(R, Arches, Test, &FnName);
 

cfe/trunk/utils/TableGen/ClangDiagnosticsEmitter.cpp

void EmitClangDiagDocs(RecordKeeper &Records, raw_ostream &OS) {
 
   // Write out the diagnostic groups.
   for (const Record *G : DiagGroups) {
     bool IsRemarkGroup = isRemarkGroup(G, DiagsInGroup);
     auto &GroupInfo = DiagsInGroup[G->getValueAsString("GroupName")];
     bool IsSynonym = GroupInfo.DiagsInGroup.empty() &&
                      GroupInfo.SubGroups.size() == 1;
 
-    writeHeader((IsRemarkGroup ? "-R" : "-W") +
-                    G->getValueAsString("GroupName"),
+    writeHeader(((IsRemarkGroup ? "-R" : "-W") +
+                    G->getValueAsString("GroupName")).str(),
                 OS);
 
     if (!IsSynonym) {
       // FIXME: Ideally, all the diagnostics in a group should have the same
       // default state, but that is not currently the case.
       auto DefaultSeverities = getDefaultSeverities(G, DiagsInGroup);
       if (!DefaultSeverities.empty() && !DefaultSeverities.count("Ignored")) {
         bool AnyNonErrors = DefaultSeverities.count("Warning") ||

cfe/trunk/utils/TableGen/ClangOptionDocEmitter.cpp

Documentation extractDocumentation(RecordKeeper &Records) {
 
   for (Record *R : Records.getAllDerivedDefinitions("Option")) {
     if (auto *A = dyn_cast<DefInit>(R->getValueInit("Alias"))) {
       Aliases[A->getDef()].push_back(R);
       continue;
     }
 
     // Pretend no-X and Xno-Y options are aliases of X and XY.
-    auto Name = R->getValueAsString("Name");
+    std::string Name = R->getValueAsString("Name");
     if (Name.size() >= 4) {
       if (Name.substr(0, 3) == "no-" && OptionsByName[Name.substr(3)]) {
         Aliases[OptionsByName[Name.substr(3)]].push_back(R);
         continue;
       }
       if (Name.substr(1, 3) == "no-" && OptionsByName[Name[0] + Name.substr(4)]) {
         Aliases[OptionsByName[Name[0] + Name.substr(4)]].push_back(R);
         continue;
if (auto *V = R->getValue(Field)) {
       if (!Value.empty())
         return Field == Primary ? Value.str() : escapeRST(Value);
     }
   }
   return StringRef();
 }
 
 void emitOptionWithArgs(StringRef Prefix, const Record *Option,
-                        ArrayRef<std::string> Args, raw_ostream &OS) {
+                        ArrayRef<StringRef> Args, raw_ostream &OS) {
   OS << Prefix << escapeRST(Option->getValueAsString("Name"));
 
   std::pair<StringRef, StringRef> Separators =
       getSeparatorsForKind(Option->getValueAsDef("Kind"));
 
   StringRef Separator = Separators.first;
   for (auto Arg : Args) {
     OS << Separator << escapeRST(Arg);
while (Args.size() < NumArgs) {
     Args.push_back(("<arg" + Twine(Args.size() + 1) + ">").str());
     // Use '--args <arg1> <arg2>...' if any number of args are allowed.
     if (Args.size() == 2 && NumArgs == UnlimitedArgs) {
       Args.back() += "...";
       break;
     }
   }
 
-  emitOptionWithArgs(Prefix, Option, Args, OS);
+  emitOptionWithArgs(Prefix, Option, std::vector<StringRef>(Args.begin(), Args.end()), OS);
 
   auto AliasArgs = Option->getValueAsListOfStrings("AliasArgs");
   if (!AliasArgs.empty()) {
     Record *Alias = Option->getValueAsDef("Alias");
     OS << " (equivalent to ";
-    emitOptionWithArgs(Alias->getValueAsListOfStrings("Prefixes").front(),
-                       Alias, Option->getValueAsListOfStrings("AliasArgs"), OS);
+    emitOptionWithArgs(
+        Alias->getValueAsListOfStrings("Prefixes").front(), Alias,
+        AliasArgs, OS);
     OS << ")";
   }
 }
 
 bool emitOptionNames(const Record *Option, raw_ostream &OS, bool EmittedAny) {
   for (auto &Prefix : Option->getValueAsListOfStrings("Prefixes")) {
     if (EmittedAny)
       OS << ", ";
void emitOption(const DocumentedOption &Option, const Record *DocInfo,
 
   // HACK: Emit a different program name with each option to work around
   // sphinx's inability to cope with options that differ only by punctuation
   // (eg -ObjC vs -ObjC++, -G vs -G=).
   std::vector<std::string> SphinxOptionIDs;
   forEachOptionName(Option, DocInfo, [&](const Record *Option) {
     for (auto &Prefix : Option->getValueAsListOfStrings("Prefixes"))
       SphinxOptionIDs.push_back(
-          getSphinxOptionID(Prefix + Option->getValueAsString("Name")));
+          getSphinxOptionID((Prefix + Option->getValueAsString("Name")).str()));
   });
   assert(!SphinxOptionIDs.empty() && "no flags for option");
   static std::map<std::string, int> NextSuffix;
   int SphinxWorkaroundSuffix = NextSuffix[*std::max_element(
       SphinxOptionIDs.begin(), SphinxOptionIDs.end(),
       [&](const std::string &A, const std::string &B) {
         return NextSuffix[A] < NextSuffix[B];
       })];

cfe/trunk/utils/TableGen/ClangSACheckersEmitter.cpp

static std::string getParentPackageFullName(const Record *R) {
   if (DefInit *DI = dyn_cast<DefInit>(R->getValueInit("ParentPackage")))
     name = getPackageFullName(DI->getDef());
   return name;
 }
 
 static std::string getPackageFullName(const Record *R) {
   std::string name = getParentPackageFullName(R);
   if (!name.empty()) name += ".";
-  return name + R->getValueAsString("PackageName");
+  name += R->getValueAsString("PackageName");
+  return name;
 }
 
 static std::string getCheckerFullName(const Record *R) {
   std::string name = getParentPackageFullName(R);
   if (isCheckerNamed(R)) {
     if (!name.empty()) name += ".";
     name += R->getValueAsString("CheckerName");
   }