[PATCH] unique_ptr with ability to get a raw pointer after release.

[David Blaikie]

On Wed, Sep 24, 2014 at 7:15 PM, Антон Ярцев <anton.yartsev at gmail.com>
wrote:

> The discussion moved to llvmdev, topic "New type of smart pointer for LLVM"
>
> ================
> Comment at: include/llvm/ADT/STLExtras.h:572
> @@ +571,3 @@
> +template<class T, class Deleter = SwitchControlledDeleter<T>>
> +class unique_ptr_deferred_release : public std::unique_ptr<T, Deleter> {
> +public:
> ----------------
> dblaikie wrote:
> > I'm sorry to go around on this a bit - but it'd be really good to just
> discuss this design for a bit (maybe no llvm-dev for increased visibility)
> as I believe we do have a need for a generic "sometimes owns" pointer
> abstraction and I'm hesitant to build a partial or awkward solution here.
> Some specific concerns (beyond the vague one above) are:
> >
> > * inheritance from unique_ptr could be very subtle/surprising -
> especially when hiding inherited functions. Taking a unique_ptr<T, ...>& to
> this unique_ptr_deferred_release and then calling release() provides
> different behavior.
> >
> > * what happens if release() is called twice? (should it be a runtime
> failure? return null? not clear - maybe null return (as you've implemented)
> makes sense)
> >
> > * there should probably be some way to both enter and exit this domain
> with unique_ptr<T, NormalDeleterHere> - so both an implicit ctor, and a
> "take()" perhaps, rather than the release(), comes back to the "what to
> return when non-owning".
> >
> > * "deferred release" doesn't seem to connote the actual use case here -
> that sounds like it might, say, "not release until the end of its lifetime,
> even when released early" which doesn't make much sense to me. This is
> really about conditional ownership - in some cases the smart pointer may
> never own the object. Nothing is deferred, it's just not handled here at
> all.
> >
> > I appreciate the work, and sorry to sort of cast doubt on it - and I
> don't mean to dissuade you from prototyping as you see fit, but I don't
> want to waste your time churning API possibilities if you'd rather wait a
> bit, have a chat about what we're trying to solve here, etc and then have a
> go at implementing (if you prefer to have that discussion and iterate on
> the API design in-code, that's fine too - but I don't want to annoy you by
> making you keep reworking your code).
> Thank you for looking at this. I agree with you, inheritance from a
> unique_ptr is not a good idea and 'deferred release' and 'release()' ain't
> a good names. I'll start a new discussion at llvm-dev.
>

Thanks!


>
> ================
> Comment at: lib/TableGen/TGParser.cpp:235
> @@ -234,3 +234,3 @@
>      // Clone the def and add it to the current multiclass
> -    Record *NewDef = new Record(**i);
> +    auto NewDef = make_unique<Record>(**i);
>
> ----------------
> dblaikie wrote:
> > This change is independent, right? Just general cleanup change that
> could be made today?
> >
> > (DefPrototypes should perhaps be a vector of unique_ptr, so that last
> line in this block can be push_back(std::move(NewDef)); ?)
> Yes, just a general cleanup, unique_ptr is sufficient here.
>

I'd be happy for you to commit that for post-commit review at this point,
if you like. (or, if you don't have commit access, I can do that for you)


>
> http://reviews.llvm.org/D5443
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140925/14043b31/attachment.html>