Thanks! Always love to see cleanup like this :) On Sat, Jan 28, 2017 at 2:35 PM Duncan P. N. Exon Smith via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> Author: dexonsmith > Date: Sat Jan 28 16:24:01 2017 > New Revision: 293395 > > URL: http://llvm.org/viewvc/llvm-project?rev=293395&view=rev > Log: > Modules: Clarify ownership of ModuleFile instances in ModuleManager, NFC > > Use std::unique_ptr to clarify the ownership of the ModuleFile instances in > ModuleManager. > > Modified: > cfe/trunk/include/clang/Serialization/ModuleManager.h > cfe/trunk/lib/Serialization/ModuleManager.cpp > > Modified: cfe/trunk/include/clang/Serialization/ModuleManager.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ModuleManager.h?rev=293395&r1=293394&r2=293395&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Serialization/ModuleManager.h (original) > +++ cfe/trunk/include/clang/Serialization/ModuleManager.h Sat Jan 28 > 16:24:01 2017 > @@ -33,7 +33,7 @@ namespace serialization { > class ModuleManager { > /// \brief The chain of AST files, in the order in which we started to > load > /// them (this order isn't really useful for anything). > - SmallVector<ModuleFile *, 2> Chain; > + SmallVector<std::unique_ptr<ModuleFile>, 2> Chain; > > /// \brief The chain of non-module PCH files. The first entry is the > one named > /// by the user, the last one is the one that doesn't depend on anything > @@ -112,12 +112,14 @@ class ModuleManager { > void returnVisitState(VisitState *State); > > public: > - typedef llvm::pointee_iterator<SmallVectorImpl<ModuleFile *>::iterator> > + typedef llvm::pointee_iterator< > + SmallVectorImpl<std::unique_ptr<ModuleFile>>::iterator> > ModuleIterator; > - typedef llvm::pointee_iterator<SmallVectorImpl<ModuleFile > *>::const_iterator> > + typedef llvm::pointee_iterator< > + SmallVectorImpl<std::unique_ptr<ModuleFile>>::const_iterator> > ModuleConstIterator; > typedef llvm::pointee_iterator< > - SmallVectorImpl<ModuleFile *>::reverse_iterator> > + SmallVectorImpl<std::unique_ptr<ModuleFile>>::reverse_iterator> > ModuleReverseIterator; > typedef std::pair<uint32_t, StringRef> ModuleOffset; > > > Modified: cfe/trunk/lib/Serialization/ModuleManager.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ModuleManager.cpp?rev=293395&r1=293394&r2=293395&view=diff > > ============================================================================== > --- cfe/trunk/lib/Serialization/ModuleManager.cpp (original) > +++ cfe/trunk/lib/Serialization/ModuleManager.cpp Sat Jan 28 16:24:01 2017 > @@ -96,30 +96,29 @@ ModuleManager::addModule(StringRef FileN > > // Check whether we already loaded this module, before > ModuleFile *ModuleEntry = Modules[Entry]; > - bool NewModule = false; > + std::unique_ptr<ModuleFile> NewModule; > if (!ModuleEntry) { > // Allocate a new module. > - NewModule = true; > - ModuleEntry = new ModuleFile(Type, Generation); > - ModuleEntry->Index = Chain.size(); > - ModuleEntry->FileName = FileName.str(); > - ModuleEntry->File = Entry; > - ModuleEntry->ImportLoc = ImportLoc; > - ModuleEntry->InputFilesValidationTimestamp = 0; > + NewModule = llvm::make_unique<ModuleFile>(Type, Generation); > + NewModule->Index = Chain.size(); > + NewModule->FileName = FileName.str(); > + NewModule->File = Entry; > + NewModule->ImportLoc = ImportLoc; > + NewModule->InputFilesValidationTimestamp = 0; > > - if (ModuleEntry->Kind == MK_ImplicitModule) { > - std::string TimestampFilename = ModuleEntry->getTimestampFilename(); > + if (NewModule->Kind == MK_ImplicitModule) { > + std::string TimestampFilename = NewModule->getTimestampFilename(); > vfs::Status Status; > // A cached stat value would be fine as well. > if (!FileMgr.getNoncachedStatValue(TimestampFilename, Status)) > - ModuleEntry->InputFilesValidationTimestamp = > + NewModule->InputFilesValidationTimestamp = > llvm::sys::toTimeT(Status.getLastModificationTime()); > } > > // Load the contents of the module > if (std::unique_ptr<llvm::MemoryBuffer> Buffer = > lookupBuffer(FileName)) { > // The buffer was already provided for us. > - ModuleEntry->Buffer = std::move(Buffer); > + NewModule->Buffer = std::move(Buffer); > } else { > // Open the AST file. > llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buf( > @@ -131,28 +130,28 @@ ModuleManager::addModule(StringRef FileN > // ModuleManager it must be the same underlying file. > // FIXME: Because FileManager::getFile() doesn't guarantee that > it will > // give us an open file, this may not be 100% reliable. > - Buf = FileMgr.getBufferForFile(ModuleEntry->File, > + Buf = FileMgr.getBufferForFile(NewModule->File, > /*IsVolatile=*/false, > /*ShouldClose=*/false); > } > > if (!Buf) { > ErrorStr = Buf.getError().message(); > - delete ModuleEntry; > return Missing; > } > > - ModuleEntry->Buffer = std::move(*Buf); > + NewModule->Buffer = std::move(*Buf); > } > > // Initialize the stream. > - ModuleEntry->Data = PCHContainerRdr.ExtractPCH(*ModuleEntry->Buffer); > + NewModule->Data = PCHContainerRdr.ExtractPCH(*NewModule->Buffer); > > // Read the signature eagerly now so that we can check it. > - if (checkSignature(ReadSignature(ModuleEntry->Data), > ExpectedSignature, ErrorStr)) { > - delete ModuleEntry; > + if (checkSignature(ReadSignature(NewModule->Data), ExpectedSignature, > ErrorStr)) > return OutOfDate; > - } > + > + // We're keeping this module. Update the map entry. > + ModuleEntry = NewModule.get(); > } else if (checkSignature(ModuleEntry->Signature, ExpectedSignature, > ErrorStr)) { > return OutOfDate; > } > @@ -175,7 +174,7 @@ ModuleManager::addModule(StringRef FileN > assert(!Modules[Entry] && "module loaded twice"); > Modules[Entry] = ModuleEntry; > > - Chain.push_back(ModuleEntry); > + Chain.push_back(std::move(NewModule)); > if (!ModuleEntry->isModule()) > PCHChain.push_back(ModuleEntry); > if (!ImportedBy) > @@ -234,11 +233,9 @@ void ModuleManager::removeModules( > // new files that will be renamed over the old ones. > if (LoadedSuccessfully.count(&*victim) == 0) > FileMgr.invalidateCache(victim->File); > - > - delete &*victim; > } > > - // Remove the modules from the chain. > + // Delete the modules. > Chain.erase(Chain.begin() + (first - begin()), > Chain.begin() + (last - begin())); > } > @@ -280,11 +277,9 @@ void ModuleManager::setGlobalIndex(Globa > > // Notify the global module index about all of the modules we've already > // loaded. > - for (unsigned I = 0, N = Chain.size(); I != N; ++I) { > - if (!GlobalIndex->loadedModuleFile(Chain[I])) { > - ModulesInCommonWithGlobalIndex.push_back(Chain[I]); > - } > - } > + for (ModuleFile &M : *this) > + if (!GlobalIndex->loadedModuleFile(&M)) > + ModulesInCommonWithGlobalIndex.push_back(&M); > } > > void ModuleManager::moduleFileAccepted(ModuleFile *MF) { > @@ -299,11 +294,7 @@ ModuleManager::ModuleManager(FileManager > : FileMgr(FileMgr), PCHContainerRdr(PCHContainerRdr), GlobalIndex(), > FirstVisitState(nullptr) {} > > -ModuleManager::~ModuleManager() { > - for (unsigned i = 0, e = Chain.size(); i != e; ++i) > - delete Chain[e - i - 1]; > - delete FirstVisitState; > -} > +ModuleManager::~ModuleManager() { delete FirstVisitState; } > > void ModuleManager::visit(llvm::function_ref<bool(ModuleFile &M)> Visitor, > llvm::SmallPtrSetImpl<ModuleFile *> > *ModuleFilesHit) { > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits