================ 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
