[PATCH] unique_ptrify ownership of ModuleFiles in the ModuleManager

[David Blaikie]

================
Comment at: include/clang/Serialization/ModuleManager.h:34-37
@@ -33,6 +33,6 @@
   /// user, the last one is the one that doesn't depend on anything further.
   SmallVector<ModuleFile *, 2> Chain;
   
   /// \brief All loaded modules, indexed by name.
-  llvm::DenseMap<const FileEntry *, ModuleFile *> Modules;
+  llvm::DenseMap<const FileEntry *, std::unique_ptr<ModuleFile>> Modules;
   
----------------
klimek wrote:
> dblaikie wrote:
> > rsmith wrote:
> > > Storing pointers to `ModuleFile`s that point into values owned by a `unique_ptr` worries me a little. Seems like there's a risk here of a dangling pointer getting left in `Chain`; I think it'd be more obvious this happened if the `delete` were explicit.
> > Not sure how much more obvious it'd be - as it stands the likely problem is a memory leak (such as 220569 ) rather than an inconsistency between these two data structures.
> > 
> > At least if something's dangling it'll likely fail pretty quick, rather than silently leak... but yeah, tradeoffs.
> > 
> > This certainly isn't the only case where we have owning and non-owning data structures at the same scope (including member scope), so I don't think it's all that surprising, but I could be wrong. Certainly we could comment the members more clearly to indicate that the poniters of one point to the same objects as the unique_ptrs of the other and that these need to be kept in step (this latter property is probably worth describing in more detail regardless)
> I'd vote for putting the unique_ptr into Chain, and having Modules point to that. I find that to be a pattern I'm using quite a bit in the new world, and at least for me personally having an explicit delete in there will help less than than the nicely visible ownership we get through unique_ptr.
Yeah, I'm totally OK with having Chains as the owner if that's any more compelling. I forget why I picked Modules - but all evidence points to Chains being the "owner" in this code (its the data structure iterated over in the dtor, iterators to Chains are what're handled in removeModules, etc).

http://reviews.llvm.org/D5980