Ping - we've hit another of these (propagating Diagnostic::OwnsDiagClient into other places) in http://llvm.org/viewvc/llvm-project?view=revision&revision=221884 Any ideas how we should be tackling this overall? I'm not entirely convinced these are fixable by design and I think we might honestly want a conditional-ownership smart pointer... But I'm happy to hold off on that a while longer if we're going to make a concerted effort to avoid propagating these half-baked solutions and actually try to remove them when we come up against problems with/around them. On Wed, Oct 8, 2014 at 1:01 PM, David Blaikie <dblaikie at gmail.com> wrote:> & repeating for the benefit of cfe-dev, here's the list of known cases of > conditional ownership (I'm sure there are others that don't fit the exact > naming convention I grep'd for): > > In Clang, simply grepping for "Owns" comes up 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 > > On Wed, Oct 8, 2014 at 1:00 PM, David Blaikie <dblaikie at gmail.com> wrote: > >> [+cfe-dev] >> >> This conversation has already been happening on llvm-dev so there's no >> good way for me to capture the entire existing discussion (so I'm jumping >> you in part-way) & the subject line could be more descriptive, but I wanted >> to add Clang developers since many of the interesting cases of conditional >> ownership I've seen were in Clang. >> >> I know some of you are also on llvm-dev but not active readers, so it >> might be worth using this as a jumping off point to go & find the full >> llvm-dev thread, read that and, when replying, add cfe-dev. >> >> If anyone not on llvm-dev wants some more context there's the email >> archive here ( >> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-September/thread.html#77136 >> ) and/or I'm happy to provide more context/summary myself. >> >> >> On Thu, Oct 2, 2014 at 9:44 AM, David Blaikie <dblaikie at gmail.com> wrote: >> >>> >>> >>> On Thu, Oct 2, 2014 at 2:43 AM, Anton Yartsev <anton.yartsev at gmail.com> >>> wrote: >>> >>>> Thanks for the feedback! >>>> >>>> >>>> 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). >>>>> >>>> David, could you, please, clarify the concept of the "maybe owning >>>> pointer"? >>>> >>> >>> See my reply to Chandler with a list of classes that hold {T*, bool} >>> members where the bool indicates whether the T* needs to be deleted or not. >>> My original goal here was to provide an API to make those more robust (more >>> precisely: to remove the need to call "release()" and allow us to stay in a >>> clear(er) owning domain). >>> >>> >>>> >>>> >>>> >>>>> 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/20141113/d6f77c6e/attachment.html>
Argyrios Kyrtzidis
2014-Nov-13 18:59 UTC
[LLVMdev] [cfe-dev] New type of smart pointer for LLVM
Could we consider moving the things you listed to shared pointer semantics ? It will be a simple and clear model; unless someone justifies that it will be a performance concern to use shared pointers there I don’t think we need a new and more complex to reason about smart pointer.> On Nov 13, 2014, at 9:42 AM, David Blaikie <dblaikie at gmail.com> wrote: > > Ping - we've hit another of these (propagating Diagnostic::OwnsDiagClient into other places) in > > http://llvm.org/viewvc/llvm-project?view=revision&revision=221884 <http://llvm.org/viewvc/llvm-project?view=revision&revision=221884> > > Any ideas how we should be tackling this overall? I'm not entirely convinced these are fixable by design and I think we might honestly want a conditional-ownership smart pointer... > > But I'm happy to hold off on that a while longer if we're going to make a concerted effort to avoid propagating these half-baked solutions and actually try to remove them when we come up against problems with/around them. > > On Wed, Oct 8, 2014 at 1:01 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote: > & repeating for the benefit of cfe-dev, here's the list of known cases of conditional ownership (I'm sure there are others that don't fit the exact naming convention I grep'd for): > > In Clang, simply grepping for "Owns" comes up 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 > > On Wed, Oct 8, 2014 at 1:00 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote: > [+cfe-dev] > > This conversation has already been happening on llvm-dev so there's no good way for me to capture the entire existing discussion (so I'm jumping you in part-way) & the subject line could be more descriptive, but I wanted to add Clang developers since many of the interesting cases of conditional ownership I've seen were in Clang. > > I know some of you are also on llvm-dev but not active readers, so it might be worth using this as a jumping off point to go & find the full llvm-dev thread, read that and, when replying, add cfe-dev. > > If anyone not on llvm-dev wants some more context there's the email archive here ( http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-September/thread.html#77136 <http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-September/thread.html#77136> ) and/or I'm happy to provide more context/summary myself. > > > On Thu, Oct 2, 2014 at 9:44 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote: > > > On Thu, Oct 2, 2014 at 2:43 AM, Anton Yartsev <anton.yartsev at gmail.com <mailto:anton.yartsev at gmail.com>> wrote: > 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"? > > See my reply to Chandler with a list of classes that hold {T*, bool} members where the bool indicates whether the T* needs to be deleted or not. My original goal here was to provide an API to make those more robust (more precisely: to remove the need to call "release()" and allow us to stay in a clear(er) owning domain). > > > >> >> 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 > > > > > _______________________________________________ > cfe-dev mailing list > cfe-dev at cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141113/1d2e7653/attachment.html>
David Blaikie
2014-Nov-13 19:25 UTC
[LLVMdev] [cfe-dev] New type of smart pointer for LLVM
On Thu, Nov 13, 2014 at 10:59 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:> Could we consider moving the things you listed to shared pointer semantics > ? It will be a simple and clear model; unless someone justifies that it > will be a performance concern to use shared pointers there I don’t think we > need a new and more complex to reason about smart pointer. >I'd generally prefer conditional ownership over shared ownership if possible - it's a narrower contract & I can still think about where the single owner is. I know in at least some of these uses, shared pointer semantics would not be applicable - TextDiagnosticPrinter::OwnsOutputStream, for example either owns its own newly allocated stream or uses std::cout (or cerr, or something) - it can never share the ownership of that stream, so it really must be "own something or own nothing". (I suppose we could use a custom no-op deleter on a shared_ptr in that case, though) But I'm open to the idea that that's the simpler/better answer than introducing a new construct - just a bit hesitant. Thanks for bringing it up as a possibility. - David> > On Nov 13, 2014, at 9:42 AM, David Blaikie <dblaikie at gmail.com> wrote: > > Ping - we've hit another of these (propagating Diagnostic::OwnsDiagClient > into other places) in > > http://llvm.org/viewvc/llvm-project?view=revision&revision=221884 > > Any ideas how we should be tackling this overall? I'm not entirely > convinced these are fixable by design and I think we might honestly want a > conditional-ownership smart pointer... > > But I'm happy to hold off on that a while longer if we're going to make a > concerted effort to avoid propagating these half-baked solutions and > actually try to remove them when we come up against problems with/around > them. > > On Wed, Oct 8, 2014 at 1:01 PM, David Blaikie <dblaikie at gmail.com> wrote: > >> & repeating for the benefit of cfe-dev, here's the list of known cases of >> conditional ownership (I'm sure there are others that don't fit the exact >> naming convention I grep'd for): >> >> In Clang, simply grepping for "Owns" comes up 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 >> >> On Wed, Oct 8, 2014 at 1:00 PM, David Blaikie <dblaikie at gmail.com> wrote: >> >>> [+cfe-dev] >>> >>> This conversation has already been happening on llvm-dev so there's no >>> good way for me to capture the entire existing discussion (so I'm jumping >>> you in part-way) & the subject line could be more descriptive, but I wanted >>> to add Clang developers since many of the interesting cases of conditional >>> ownership I've seen were in Clang. >>> >>> I know some of you are also on llvm-dev but not active readers, so it >>> might be worth using this as a jumping off point to go & find the full >>> llvm-dev thread, read that and, when replying, add cfe-dev. >>> >>> If anyone not on llvm-dev wants some more context there's the email >>> archive here ( >>> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-September/thread.html#77136 >>> ) and/or I'm happy to provide more context/summary myself. >>> >>> >>> On Thu, Oct 2, 2014 at 9:44 AM, David Blaikie <dblaikie at gmail.com> >>> wrote: >>> >>>> >>>> >>>> On Thu, Oct 2, 2014 at 2:43 AM, Anton Yartsev <anton.yartsev at gmail.com> >>>> wrote: >>>> >>>>> Thanks for the feedback! >>>>> >>>>> >>>>> 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). >>>>>> >>>>> David, could you, please, clarify the concept of the "maybe owning >>>>> pointer"? >>>>> >>>> >>>> See my reply to Chandler with a list of classes that hold {T*, bool} >>>> members where the bool indicates whether the T* needs to be deleted or not. >>>> My original goal here was to provide an API to make those more robust (more >>>> precisely: to remove the need to call "release()" and allow us to stay in a >>>> clear(er) owning domain). >>>> >>>> >>>>> >>>>> >>>>> >>>>>> 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 >>>>> >>>>> >>>> >>> >> > _______________________________________________ > cfe-dev mailing list > cfe-dev at cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141113/673c7fb0/attachment.html>