================ 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: > 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). Found some old git branches I had lying around including this one.
Turns out one reason making Chain the owner is a bit harder is that it's exposed in more places (ModuleManager exposes iterators to Chain through begin/end/etc). This goes into many places, including llvm Graph APIs (llvm/Support/GraphWriter, et al), other places like ModuleManager.cpp:186: llvm::SmallPtrSet<ModuleFile *, 4> victimSet(first, last); So to have Chain as the owner, we'd probably need to implement an iterator adapter from iterator over unique_ptr<T> to iterator over T* rvalue (since there's no underlying T* to point to/reference). Not sure if that's worth it here, but we'll probably eventually want such an abstraction, so maybe this can wait until I find the time/inclination to implement such a thing. http://reviews.llvm.org/D5980 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
