[PATCH] D15251: [Clang] Use range loops and autos in lib/Serialization/ASTReader.cpp

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: lib/Serialization/ASTReader.cpp:3390
@@ -3393,5 +3389,3 @@
     Mod->getExportedModules(Exports);
-    for (SmallVectorImpl<Module *>::iterator
-           I = Exports.begin(), E = Exports.end(); I != E; ++I) {
-      Module *Exported = *I;
+    for (const auto &Exported : Exports) {
       if (Visited.insert(Exported).second)
----------------
hans wrote:
>   for (Module *Exported : Exports)
> 
> would be much easier to read. (And having Exported by a reference to a pointer seems odd.)
> for (Module *Exported : Exports)
> would be much easier to read. (And having Exported by a reference to a pointer seems odd.)

I would also find: for (const auto *Exported : Exports) to be fine, but definitely agree with Hans that const auto & isn't what we want here.


================
Comment at: lib/Serialization/ASTReader.cpp:3512
@@ -3517,5 +3511,3 @@
   // Load the AST blocks of all of the modules that we loaded.
-  for (SmallVectorImpl<ImportedModule>::iterator M = Loaded.begin(),
-                                              MEnd = Loaded.end();
-       M != MEnd; ++M) {
-    ModuleFile &F = *M->Mod;
+  for (const auto &M : Loaded) {
+    ModuleFile &F = *M.Mod;
----------------
hans wrote:
> Using a range-based for loop here is very nice, but again I'm not sure the "auto" is helping.
> 
>   for (const auto &ImportedModule : ...
> 
> would be easier to read, I think.
> Using a range-based for loop here is very nice, but again I'm not sure the "auto" is helping.
>
> for (const auto &ImportedModule : ...
> would be easier to read, I think.

I think const auto &M is reasonable (it fits with our usual naming conventions), but wouldn't complain if there were a more descriptive name used.


================
Comment at: lib/Serialization/ASTReader.cpp:4306
@@ -4317,3 +4305,3 @@
         case INPUT_FILE:
-          bool Overridden = static_cast<bool>(Record[3]);
+          auto Overridden = static_cast<bool>(Record[3]);
           std::string Filename = Blob;
----------------
hans wrote:
> I'd say "bool" is a better choice here.
> I'd say "bool" is a better choice here.

Agreed.

================
Comment at: lib/Serialization/ASTReader.cpp:5074
@@ -5087,3 +5073,3 @@
     bool operator()(ModuleFile &M) {
-      HeaderFileInfoLookupTable *Table
+      auto *Table
         = static_cast<HeaderFileInfoLookupTable *>(M.HeaderFileInfoTable);
----------------
If auto doesn't shorten this sufficiently to fit on a single line, I think I slightly prefer HeaderFileInfoLookupTable. It's easier to figure out what Table is without having to go to another line in the source file to find out.

================
Comment at: lib/Serialization/ASTReader.cpp:5104
@@ -5117,3 +5103,3 @@
   SmallVector<DiagnosticsEngine::DiagState *, 32> DiagStates;
-  for (ModuleIterator I = ModuleMgr.begin(), E = ModuleMgr.end(); I != E; ++I) {
-    ModuleFile &F = *(*I);
+  for (const auto I : ModuleMgr) {
+    ModuleFile &F = *I;
----------------
hans wrote:
> I'd spell out ModuleIterator instead of auto here.
> I'd spell out ModuleIterator instead of auto here.

I don't see a reason to do that, but I also don't like that this appears to be doing a copy. Is ModuleIterator a pointer? If so, this should be const auto *I instead. If not, const auto &.

================
Comment at: lib/Serialization/ASTReader.cpp:6860
@@ -6878,5 +6859,3 @@
   llvm::errs() << "\n*** PCH/Modules Loaded:";
-  for (ModuleManager::ModuleConstIterator M = ModuleMgr.begin(), 
-                                       MEnd = ModuleMgr.end();
-       M != MEnd; ++M)
-    (*M)->dump();
+  for (const auto M : ModuleMgr)
+    M->dump();
----------------
hans wrote:
> Maybe:
> 
>   for (const ModuleFile &MF : ModuleMgr)
> Maybe:
> 
> for (const ModuleFile &MF : ModuleMgr)

I don't think that naming the type adds benefit for range-based for loop iteration variables unless the type is really obscure. We don't name the types in the vast majority of places in the code base from what I can tell.

================
Comment at: lib/Serialization/ASTReader.cpp:7220
@@ -7240,2 +7219,3 @@
+    auto *D
       = dyn_cast_or_null<TypedefNameDecl>(GetDecl(ExtVectorDecls[I]));
     if (D)
----------------
hans wrote:
> Does this fit on one line now?
> Does this fit on one line now?

If it does not, I don't think auto is the correct type specifier to use here.


Repository:
  rL LLVM

http://reviews.llvm.org/D15251