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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:237
   OS << "#define COMMA ,\n";
-  for (PrefixesT::const_iterator I = Prefixes.begin(), E = Prefixes.end();
-                                  I != E; ++I) {
+  for (auto PrefixIt : Prefixes) {
     OS << "PREFIX(";
----------------
- 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.


================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:245-247
+    for (const SmallString<2> &PrefixKey : PrefixIt.first) {
+      OS << "\"" << PrefixKey << "\" COMMA ";
     }
----------------
- 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.


================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:256
   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.
----------------
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.


================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:341
       OS << "\"";
-      for (size_t i = 0, e = AliasArgs.size(); i != e; ++i)
-        OS << AliasArgs[i] << "\\0";
+      for (const StringRef &AliasArg : AliasArgs)
+        OS << AliasArg << "\\0";
----------------
`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).


================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:392
   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.
----------------
As above, you can use `make_pointee_range`.


================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:458
   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")))
----------------
`make_pointee_range`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93526