On Wed, Sep 24, 2014 at 7:50 PM, Justin Bogner <mail at justinbogner.com> wrote:> Anton Yartsev <anton.yartsev at gmail.com> writes: > > Hello everyone, > > > > I bring to discussion the necessity/design of a new type of smart > pointer. > > r215176 and r217791 rise the problem, D5443 is devoted to the solution. > > r215176 applies several temporary ugly fixes of memory leaks in > TGParser.cpp > > which would be great to be refactored using smart pointers. D5443 > demonstrates > > how the solution with a certain type of smart pointer would look like > (see > > changes in TGParser::ParseDef(), TGParser::InstantiateMulticlassDef() and > > TGParser::ParseSimpleValue()). > > > > Briefly: > > consider a leaky example: > > { > > T* p = new T; > > if (condition1) { > > f(p); // takes ownership of p > > } > > p->SomeMethod(); > > > > if (condition2) { > > return nullptr; // Leak! > > } > > > > g(p); // don't take ownership of p > > return p; > > } > > > > The preferred solution would look like: > > { > > smart_ptr<T> p(new T); > > if (condition1) { > > f(p.StopOwn()); // takes ownership of p > > So this takes ownership, but promises not to destroy the pointee in some > way? > > > } > > p->SomeMethod(); > > > > if (condition2) { > > return nullptr; // > > I guess p is sometimes destroyed here, depending on condition1? > > > } > > > > g(p.Get()); // don't take ownership of p > > return p.StopOwn(); > > } > > What does it mean to stop owning the pointer twice? Doesn't this leak p > in the case where condition1 was false? > > > Neither unique_ptr nor shared_ptr can be used in the place of smart_ptr > as > > unique_ptr sets the raw pointer to nullptr after release() (StopOwn() in > the > > example above) whereas shared_ptr is unable to release. > > I don't understand why shared_ptr wouldn't suffice for the example > above. There are two cases in your example where you try to release the > pointer - in one of them it seems you don't actually want to release it, > because you continue using it after, and in the other the scope ends > immediately after the release. The shared_ptr releases when it goes out > of scope, so it seems to be exactly what you want here. > > Maybe I'm just misunderstanding the use case here, but I fear that a > type like this one would just serve to paper over problems where the > ownership is very unclear, without really helping much. >Perhaps this is a case of poor examples, I'm not sure. I certainly share your concern that this might not be the right path forward, but it's something that I've seen come up repeatedly when doing unique_ptr migrations, perhaps moreso in Clang's frontend code. Essentially there are a variety of data structures that "sometimes" own their underlying data. Often implemented as a raw pointer and a boolean. (sometimes I've migrated this to a raw pointer and a unique_ptr - the raw pointer always points to the thing of interest, the unique_ptr is sometimes non-null and points to the same thing to keep it alive (or destroy it when it's no longer needed, more specifically)) I can go & dredge up some examples if we want to discuss the particular merits & whether each of those cases would be better solved in some other way, but it seemed pervasive enough in the current codebase that some incremental improvement could be gained by replacing these cases with a more specific tool for the job. We might still consider this tool to be "not ideal".> > > Attached is a scratch that illustrates how the minimal > > API/implementation of a desired smart pointer sufficient for > > refactoring would look like. Any ideas and suggestions are > > appreciated. > > > > //===-- CondOwnershipPtr.h - Smart ptr with conditional release -*- C++ > -*-===// > > // > > // The LLVM Compiler Infrastructure > > // > > // This file is distributed under the University of Illinois Open Source > > // License. See LICENSE.TXT for details. > > > //===----------------------------------------------------------------------===// > > > > #ifndef LLVM_ADT_CONDOWNERSHIPPTR_H > > #define LLVM_ADT_CONDOWNERSHIPPTR_H > > > > namespace llvm { > > > > template<class T> > > class CondOwnershipPtr { > > T* Ptr; > > bool Own; > > > > void Delete() { > > if (Ptr && !Own) > > delete Ptr; > > } > > This seems to delete the pointer iff we *don't* own it. I think you have > that backwards... > > > public: > > CondOwnershipPtr() : Ptr(nullptr), Own(true) {} > > explicit CondOwnershipPtr(T* p) : Ptr(p), Own(true) {} > > > > ~CondOwnershipPtr() { > > Delete(); > > } > > > > T* Get() const { > > return Ptr; > > } > > > > T* StopOwn() const { > > Own = false; > > return Ptr; > > } > > > > void Reset(T* P = nullptr) { > > if (P != Ptr) { > > Delete(); > > Ptr = P; > > Own = true; > > } > > } > > > > bool Owns() const { > > return Own; > > } > > > > operator bool() const { > > return Ptr != nullptr; > > } > > > > T& operator*() const { > > return *Ptr; > > } > > > > T* operator->() const { > > return Ptr; > > } > > }; > > > > } // end namespace llvm > > > > #endif > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140924/4cbbce90/attachment.html>
On 25 September 2014 06:16, David Blaikie <dblaikie at gmail.com> wrote:> I can go & dredge up some examples if we want to discuss the particular > merits & whether each of those cases would be better solved in some other > way, but it seemed pervasive enough in the current codebase that some > incremental improvement could be gained by replacing these cases with a more > specific tool for the job. We might still consider this tool to be "not > ideal".Having done that in another code base, I can see both merits and problems. Our smart pointer behaved more or less like it's described above (explicit acquire/release) and we've seen around 20% performance improvement over shared_ptr due to savings on acquire/release semantics. We had three derivative types of pointers (Shared/Owned/Linked) which would just differ on the default behaviour of construction / destruction, but all of them could still explicitly call getClear / getLink / etc. The downside was that almost every interaction with smart pointers had to be carefully planned and there was a lot of room for errors, especially from people that didn't know the context. In the end, the amount of work that had to be put to make it work was similar than when dealing with normal pointers, but you had to learn yet another pointer semantics. The marginal gain was that pointer interactions were explicit, making it easier for someone *not* used to C++ pointer semantics to understand when reading code, not necessarily when writing it. The marginal lost was getting people that already knew the STL and Boost smart pointer semantics to get confused. Having done that, I still rather use normal pointers and have valgrind / sanitizers tell me when I screwed up. My tuppence. cheers, --renato
On Thu, Sep 25, 2014 at 1:44 AM, Renato Golin <renato.golin at linaro.org> wrote:> On 25 September 2014 06:16, David Blaikie <dblaikie at gmail.com> wrote: > > I can go & dredge up some examples if we want to discuss the particular > > merits & whether each of those cases would be better solved in some other > > way, but it seemed pervasive enough in the current codebase that some > > incremental improvement could be gained by replacing these cases with a > more > > specific tool for the job. We might still consider this tool to be "not > > ideal". > > Having done that in another code base, I can see both merits and problems. > > Our smart pointer behaved more or less like it's described above > (explicit acquire/release)I'm not quite sure I follow you (or that you're following me). The goal isn't about being explicit about acquire/release (indeed I wouldn't mind more implicit acquisition from an always-owning pointer (unique_ptr) to a sometimes-owning pointer (whatever we end up calling it), though release is always going to be explicit I suspect).> and we've seen around 20% performance > improvement over shared_ptr due to savings on acquire/release > semantics.The existing use cases I'm interested in aren't using shared_ptr and wouldn't be ideally migrated to it (in some cases the existing ownership might be on the stack (so it can't be shared) or several layers up the call stack through which raw pointers have been passed (eg: build something, pass it through a few APIs, underlying thing 'wants' to take ownership, but it will be constructed/destroyed before this API returns - so we hack around it either by having a "OwnsThing" flag in the underlying thing, or having a "takeThing" we hope we call before the underlying thing dies and destroys the thing)). We're dealing with types that have raw pointer + bool indicating ownership members or worse, types which take ownership but we lie to about giving them ownership (so some API takes a non-owning T* (or T&), passes it as owning to some other thing, then is sure to take ownership back from that thing and call 'release()" on the unique_ptr to ensure ownership was never really taken).> We had three derivative types of pointers > (Shared/Owned/Linked) which would just differ on the default behaviour > of construction / destruction, but all of them could still explicitly > call getClear / getLink / etc. > > The downside was that almost every interaction with smart pointers had > to be carefully planned and there was a lot of room for errors, >My hope is that having a single construct for conditional ownership will be less confusing than the existing ad-hoc solutions in many places. It's possible that the right answer is to remove the conditional ownership in these APIs entirely, but I'm not sure that's possible/practical/desired (it might be - which is why I've been hesitant to write this abstraction myself just yet - sort of letting it stew in my head a bit to see what feels right).> especially from people that didn't know the context. In the end, the > amount of work that had to be put to make it work was similar than > when dealing with normal pointers,I rather hope that's not the case - these conditionally owning raw pointers are pretty subtle, easy to miss a delete and leak, easy to have an early return and fail to take back and release ownership from something that wasn't really owning in the first place, etc.> but you had to learn yet another > pointer semantics. > > The marginal gain was that pointer interactions were explicit, making > it easier for someone *not* used to C++ pointer semantics to > understand when reading code, not necessarily when writing it. The > marginal lost was getting people that already knew the STL and Boost > smart pointer semantics to get confused. >This is another pointer semantic - I'm not suggesting replacing unique_ptr or shared_ptr - I want to use them as much as possible where they represent the right semantic. I'm just dealing with a situation that doesn't map directly to either of those: conditional ownership.> > Having done that, I still rather use normal pointers and have valgrind > / sanitizers tell me when I screwed up. > > My tuppence. > > cheers, > --renato >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140925/84124a3c/attachment.html>