[TableGen] NFC: Switch to range-based for loops in OptParserEmitter

[jansvoboda11]

Summary

This simplifies the code a bit. No functionality change.

[dexonsmith]

Thanks for the cleanup! I have a few nits below, some of which will help to reduce the diff. LGTM once you fix those.

llvm/utils/TableGen/OptParserEmitter.cpp

237

• This name is confusing to me because PrefixIt doesn't seem to be an iterator (but It suggests it is). Can this be Prefix instead? Or maybe P?
• Should this be const auto & to avoid the copy? If not, please spell out the type so it's easy to confirm at the call site that a copy is cheap enough.

245–247

• You might consider dropping the braces since you're touching this code anyway.
• I think the type here could be a by-value StringRef to avoid having to change it if the string storage implementation changes in the future.

256

You can reduce the diff with:

for (const Record &R : llvm::make_pointee_range(Groups)) {

A side benefit is we keep a non-non-null variable from looking like it could be null to the casual reader of later lines.

341

StringRef is cheap to copy; just use it by-value. Generally we try to avoid taking it by-reference (unless it's being modified, or if it somehow avoids an #include although it's so pervasive that's probably not worth doing).

392

As above, you can use make_pointee_range.

458

make_pointee_range

[jansvoboda11]

Thanks for the suggestions! llvm::make_pointee_range is handy.

[jansvoboda11]

Incorporate review feedback

llvm/utils/TableGen/OptParserEmitter.cpp

 //===- OptParserEmitter.cpp - Table Driven Command Line Parsing -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
void EmitOptParser(RecordKeeper &Records, raw_ostream &OS) {
 
   array_pod_sort(Opts.begin(), Opts.end(), CompareOptionRecords);
   // Generate prefix groups.
   typedef SmallVector<SmallString<2>, 2> PrefixKeyT;
   typedef std::map<PrefixKeyT, std::string> PrefixesT;
   PrefixesT Prefixes;
   Prefixes.insert(std::make_pair(PrefixKeyT(), "prefix_0"));
   unsigned CurPrefix = 0;
-  for (unsigned i = 0, e = Opts.size(); i != e; ++i) {
-    const Record &R = *Opts[i];
-    std::vector<StringRef> prf = R.getValueAsListOfStrings("Prefixes");
+  for (const Record *R : Opts) {
+    std::vector<StringRef> prf = R->getValueAsListOfStrings("Prefixes");
     PrefixKeyT prfkey(prf.begin(), prf.end());
     unsigned NewPrefix = CurPrefix + 1;
     if (Prefixes.insert(std::make_pair(prfkey, (Twine("prefix_") +
                                               Twine(NewPrefix)).str())).second)
       CurPrefix = NewPrefix;
   }
 
   // Dump prefixes.
 
   OS << "/////////\n";
   OS << "// Prefixes\n\n";
   OS << "#ifdef PREFIX\n";
   OS << "#define COMMA ,\n";
-  for (PrefixesT::const_iterator I = Prefixes.begin(), E = Prefixes.end();
-                                  I != E; ++I) {
+  for (auto PrefixIt : Prefixes) {
     OS << "PREFIX(";
 
     // Prefix name.
-    OS << I->second;
+    OS << PrefixIt.second;
 
     // Prefix values.
     OS << ", {";
-    for (PrefixKeyT::const_iterator PI = I->first.begin(),
-                                    PE = I->first.end(); PI != PE; ++PI) {
-      OS << "\"" << *PI << "\" COMMA ";
+    for (const SmallString<2> &PrefixKey : PrefixIt.first) {
+      OS << "\"" << PrefixKey << "\" COMMA ";
     }
     OS << "nullptr})\n";
   }
   OS << "#undef COMMA\n";
   OS << "#endif // PREFIX\n\n";
 
   OS << "/////////\n";
   OS << "// Groups\n\n";
   OS << "#ifdef OPTION\n";
-  for (unsigned i = 0, e = Groups.size(); i != e; ++i) {
-    const Record &R = *Groups[i];
-
+  for (const Record *R : Groups) {
     // Start a single option entry.
     OS << "OPTION(";
 
     // The option prefix;
     OS << "nullptr";
 
     // The option string.
-    OS << ", \"" << R.getValueAsString("Name") << '"';
+    OS << ", \"" << R->getValueAsString("Name") << '"';
 
     // The option identifier name.
-    OS << ", " << getOptionName(R);
+    OS << ", " << getOptionName(*R);
 
     // The option kind.
     OS << ", Group";
 
     // The containing option group (if any).
     OS << ", ";
-    if (const DefInit *DI = dyn_cast<DefInit>(R.getValueInit("Group")))
+    if (const DefInit *DI = dyn_cast<DefInit>(R->getValueInit("Group")))
       OS << getOptionName(*DI->getDef());
     else
       OS << "INVALID";
 
     // The other option arguments (unused for groups).
     OS << ", INVALID, nullptr, 0, 0";
 
     // The option help text.
-    if (!isa<UnsetInit>(R.getValueInit("HelpText"))) {
+    if (!isa<UnsetInit>(R->getValueInit("HelpText"))) {
       OS << ",\n";
       OS << "       ";
-      write_cstring(OS, R.getValueAsString("HelpText"));
+      write_cstring(OS, R->getValueAsString("HelpText"));
     } else
       OS << ", nullptr";
 
     // The option meta-variable name (unused).
     OS << ", nullptr";
 
     // The option Values (unused for groups).
     OS << ", nullptr)\n";
auto WriteOptRecordFields = [&](raw_ostream &OS, const Record &R) {
     // would become "foo\0bar\0". Note that the compiler adds an implicit
     // terminating \0 at the end.
     OS << ", ";
     std::vector<StringRef> AliasArgs = R.getValueAsListOfStrings("AliasArgs");
     if (AliasArgs.size() == 0) {
       OS << "nullptr";
     } else {
       OS << "\"";
-      for (size_t i = 0, e = AliasArgs.size(); i != e; ++i)
-        OS << AliasArgs[i] << "\\0";
+      for (const StringRef &AliasArg : AliasArgs)
+        OS << AliasArg << "\\0";
       OS << "\"";
     }
 
     // The option flags.
     OS << ", ";
     int NumFlags = 0;
     const ListInit *LI = R.getValueAsListInit("Flags");
     for (Init *I : *LI)
void EmitOptParser(RecordKeeper &Records, raw_ostream &OS) {
   };
 
   auto IsMarshallingOption = [](const Record &R) {
     return !isa<UnsetInit>(R.getValueInit("KeyPath")) &&
            !R.getValueAsString("KeyPath").empty();
   };
 
   std::vector<const Record *> OptsWithMarshalling;
-  for (unsigned I = 0, E = Opts.size(); I != E; ++I) {
-    const Record &R = *Opts[I];
-
+  for (const Record *R : Opts) {
     // Start a single option entry.
     OS << "OPTION(";
-    WriteOptRecordFields(OS, R);
+    WriteOptRecordFields(OS, *R);
     OS << ")\n";
-    if (IsMarshallingOption(R))
-      OptsWithMarshalling.push_back(&R);
+    if (IsMarshallingOption(*R))
+      OptsWithMarshalling.push_back(R);
   }
   OS << "#endif // OPTION\n";
 
   auto CmpMarshallingOpts = [](const Record *const *A, const Record *const *B) {
     unsigned AID = (*A)->getID();
     unsigned BID = (*B)->getID();
 
     if (AID < BID)
void EmitOptParser(RecordKeeper &Records, raw_ostream &OS) {
 
   OS << "#endif // SIMPLE_ENUM_VALUE_TABLE\n";
   OS << "\n";
 
   OS << "\n";
   OS << "#ifdef OPTTABLE_ARG_INIT\n";
   OS << "//////////\n";
   OS << "// Option Values\n\n";
-  for (unsigned I = 0, E = Opts.size(); I != E; ++I) {
-    const Record &R = *Opts[I];
-    if (isa<UnsetInit>(R.getValueInit("ValuesCode")))
+  for (const Record *R : Opts) {
+    if (isa<UnsetInit>(R->getValueInit("ValuesCode")))
       continue;
     OS << "{\n";
     OS << "bool ValuesWereAdded;\n";
-    OS << R.getValueAsString("ValuesCode");
+    OS << R->getValueAsString("ValuesCode");
     OS << "\n";
-    for (StringRef Prefix : R.getValueAsListOfStrings("Prefixes")) {
+    for (StringRef Prefix : R->getValueAsListOfStrings("Prefixes")) {
       OS << "ValuesWereAdded = Opt.addValues(";
       std::string S(Prefix);
-      S += R.getValueAsString("Name");
+      S += R->getValueAsString("Name");
       write_cstring(OS, S);
       OS << ", Values);\n";
       OS << "(void)ValuesWereAdded;\n";
       OS << "assert(ValuesWereAdded && \"Couldn't add values to "
             "OptTable!\");\n";
     }
     OS << "}\n";
   }
   OS << "\n";
   OS << "#endif // OPTTABLE_ARG_INIT\n";
 }
 } // end namespace llvm