On Wed, Oct 1, 2014 at 3:14 PM, Anton Yartsev <anton.yartsev at gmail.com> wrote:> Ping! > > Suggested is a wrapper over a raw pointer that is intended for freeing > wrapped memory at the end of wrappers lifetime if ownership of a raw > pointer was not taken away during the lifetime of the wrapper. > The main difference from unique_ptr is an ability to access the wrapped > pointer after the ownership is transferred. > To make the ownership clearer the wrapper is designed for local-scope > usage only. >I'm still concerned this isn't the right direction, and that we instead want a more formal "maybe owning pointer". The specific use case you've designed for here, in my experience, doesn't seem to come up often enough to justify a custom ADT - but the more general tool of sometimes-owning smart pointer seems common enough (& problematic enough) to warrant a generic data structure (and such a structure would also be appliable to the TGParser use case where this came up). I'd love to hear some more opinions, but maybe we're not going to get them...> > The main goal of the wrapper is to eliminate leaks like those in TGParser.cpp > - r215176 > <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?r1=215176&r2=215175&pathrev=215176&diff_format=f>. > With the wrapper the fixes applied at r215176 > <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?r1=215176&r2=215175&pathrev=215176&diff_format=f> could > be refactored in the more easy and safe way. > > Attached is a proposed interface/implementation. > Any ideas, suggestions? Is it OK to move forward with the solution? > > Hello everyone, > > I bring to discussion the necessity/design of a new type of smart pointer. > r215176 and r217791 rise the problem, D5443 > <http://reviews.llvm.org/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 > <http://reviews.llvm.org/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 > } > p->SomeMethod(); > > if (condition2) { > return nullptr; // > } > > g(p.Get()); // don't take ownership of p > return p.StopOwn(); > } > > 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. > > 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. > > -- > Anton > > > > -- > Anton > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141001/55f9601f/attachment.html>
On Wed, Oct 1, 2014 at 3:36 PM, David Blaikie <dblaikie at gmail.com> wrote:> On Wed, Oct 1, 2014 at 3:14 PM, Anton Yartsev <anton.yartsev at gmail.com> > wrote: > >> Ping! >> >> Suggested is a wrapper over a raw pointer that is intended for freeing >> wrapped memory at the end of wrappers lifetime if ownership of a raw >> pointer was not taken away during the lifetime of the wrapper. >> The main difference from unique_ptr is an ability to access the wrapped >> pointer after the ownership is transferred. >> To make the ownership clearer the wrapper is designed for local-scope >> usage only. >> > > I'm still concerned this isn't the right direction, and that we instead > want a more formal "maybe owning pointer". The specific use case you've > designed for here, in my experience, doesn't seem to come up often enough > to justify a custom ADT - but the more general tool of sometimes-owning > smart pointer seems common enough (& problematic enough) to warrant a > generic data structure (and such a structure would also be appliable to the > TGParser use case where this came up). > > I'd love to hear some more opinions, but maybe we're not going to get > them... >I strongly agree that the example here isn't compelling. I think it is a very fundamental design problem when there is a need for a pointer value after it has been deallocated... I even question whether we need a "maybe owning" smart pointer, or whether this is just an indication that the underlying data structure is *wrong*. The idea of "maybe" and "owning" going to gether, in any sense, seems flat out broken to me from a design perspective, and so I'm not enthused about providing abstractions that make it easier to build systems with unclear ownership semantics. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141001/307211fb/attachment.html>
Thanks for the feedback!> > On Wed, Oct 1, 2014 at 3:36 PM, David Blaikie <dblaikie at gmail.com > <mailto:dblaikie at gmail.com>> wrote: > > On Wed, Oct 1, 2014 at 3:14 PM, Anton Yartsev > <anton.yartsev at gmail.com <mailto:anton.yartsev at gmail.com>> wrote: > > Ping! > > Suggested is a wrapper over a raw pointer that is intended for > freeing wrapped memory at the end of wrappers lifetime if > ownership of a raw pointer was not taken away during the > lifetime of the wrapper. > The main difference from unique_ptr is an ability to access > the wrapped pointer after the ownership is transferred. > To make the ownership clearer the wrapper is designed for > local-scope usage only. > > > I'm still concerned this isn't the right direction, and that we > instead want a more formal "maybe owning pointer". The specific > use case you've designed for here, in my experience, doesn't seem > to come up often enough to justify a custom ADT - but the more > general tool of sometimes-owning smart pointer seems common enough > (& problematic enough) to warrant a generic data structure (and > such a structure would also be appliable to the TGParser use case > where this came up). >David, could you, please, clarify the concept of the "maybe owning pointer"?> > I'd love to hear some more opinions, but maybe we're not going to > get them... > > > I strongly agree that the example here isn't compelling. > > I think it is a very fundamental design problem when there is a need > for a pointer value after it has been deallocated...Not deallocated but stored to the long-living storage. I agree, the problem is in design, the suggested wrapper is an intermediate solution, it was just designed to replace the existing ugly fixes.> I even question whether we need a "maybe owning" smart pointer, or > whether this is just an indication that the underlying data structure > is *wrong*. The idea of "maybe" and "owning" going to gether, in any > sense, seems flat out broken to me from a design perspective, and so > I'm not enthused about providing abstractions that make it easier to > build systems with unclear ownership semantics.-- Anton -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141002/c6a9d70e/attachment.html>
Duncan P. N. Exon Smith
2014-Oct-02 16:11 UTC
[LLVMdev] New type of smart pointer for LLVM
> On Oct 1, 2014, at 3:53 PM, Chandler Carruth <chandlerc at google.com> wrote: > > I even question whether we need a "maybe owning" smart pointer, or whether this is just an indication that the underlying data structure is *wrong*. The idea of "maybe" and "owning" going to gether, in any sense, seems flat out broken to me from a design perspective,I absolutely agree with this.> and so I'm not enthused about providing abstractions that make it easier to build systems with unclear ownership semantics.We have a number of APIs that use this model, without abstractions. To the extent that providing the abstraction encourages the model, it's dangerous. But if it helps to clarify the code that's already there and facilitates a transition to better models, then it might be worthwhile anyway.
On Wed, Oct 1, 2014 at 3:53 PM, Chandler Carruth <chandlerc at google.com> wrote:> > On Wed, Oct 1, 2014 at 3:36 PM, David Blaikie <dblaikie at gmail.com> wrote: > >> On Wed, Oct 1, 2014 at 3:14 PM, Anton Yartsev <anton.yartsev at gmail.com> >> wrote: >> >>> Ping! >>> >>> Suggested is a wrapper over a raw pointer that is intended for freeing >>> wrapped memory at the end of wrappers lifetime if ownership of a raw >>> pointer was not taken away during the lifetime of the wrapper. >>> The main difference from unique_ptr is an ability to access the wrapped >>> pointer after the ownership is transferred. >>> To make the ownership clearer the wrapper is designed for local-scope >>> usage only. >>> >> >> I'm still concerned this isn't the right direction, and that we instead >> want a more formal "maybe owning pointer". The specific use case you've >> designed for here, in my experience, doesn't seem to come up often enough >> to justify a custom ADT - but the more general tool of sometimes-owning >> smart pointer seems common enough (& problematic enough) to warrant a >> generic data structure (and such a structure would also be appliable to the >> TGParser use case where this came up). >> >> I'd love to hear some more opinions, but maybe we're not going to get >> them... >> > > I strongly agree that the example here isn't compelling. > > I think it is a very fundamental design problem when there is a need for a > pointer value after it has been deallocated... >The use case here wasn't so much needing the pointer value after its been deallocated, but after ownership has been passed. Essentially: auto p = make_unique<T>(...); addThing(std::move(p)); /* do more things with *p here... */ so you end up writing code like this: auto pOwner = make_unique... auto p = pOwner.get(); addThing(std::move(pOwner) /* do things with *p here... */ Or you have the opposite: std::unique_ptr<T> owner; T *t; if (x) t = &defaultT; else { owner = make_unique... t = owner.get(); } /* stuff with t here... */> I even question whether we need a "maybe owning" smart pointer, or whether > this is just an indication that the underlying data structure is *wrong*. > The idea of "maybe" and "owning" going to gether, in any sense, seems flat > out broken to me from a design perspective, and so I'm not enthused about > providing abstractions that make it easier to build systems with unclear > ownership semantics. >Yep - this is the big open question for me & the dialog I really want to have about this (& why I've put off writing a maybe owning smart pointer so far). As I transform things to unique_ptr I tend to punt on any of the 'hard' ones, and this is one of them - so the ownership semantics non-unique_ptr-ified these days are becoming more and more these leftover cases. The clearest example of such an API is something like Module... Duncan linked to a thread on that issue. But I see a lot of it in Clang, simply grepping for "Owns" comesup with the following boolean members: CodeGenAction::OwnsVMContext ASTReader::OwnsDeserializationListener Diagnostic::OwnsDiagClient Preprocessor::OwnsHeaderSearch TokenLexer::OwnsTokens Action::OwnsInputs (this ones trickier - it's a boolean that indicates whether all the elements of a vector<T*> are owned or unowned) ASTUnit::OwnsRemappedFileBuffers VerifyDiagnosticConsumer::OwnsPrimaryClient TextDiagnosticPrinter::OwnsOutputStream FixItRewriter::OwnsClient Tooling::OwnsAction Some in LLVM: circular_raw_ostream::OwnsStream Arg::OwnsValues (another tricky one with a bool flag and a vector of raw pointers, if I recall correctly) And a couple that I changed {T*, bool} to {T*, unique_ptr<T>}: LogDiagnosticPrinter::StreamOwner ASTUnit::ComputedPreamble::Owner -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141002/b300ad47/attachment.html>