[PATCH] unique_ptrify ownership of ModuleFiles in the ModuleManager

[Manuel Klimek]

================
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;
   
----------------
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.

http://reviews.llvm.org/D5980