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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
