Page MenuHomePhabricator

unique_ptr with ability to get a raw pointer after release.
Needs ReviewPublic

Authored by ayartsev on Sep 22 2014, 11:12 AM.

Details

Reviewers
dblaikie
Summary

The work is inspired by r215176 and comment to r217791.
unique_ptr_deferred_release is intended for use in cases when conditional releases are mixed with raw pointer access, e.g:

unique_ptr_deferred_release<Type> P(new P);
if (cond) {
  P.deferred_release();
}
assert(P.get()); // 'true' regardless of whether deferred_release() was called or not.
NonOwningFunction(P.get());

Please review!

Diff Detail

Event Timeline

ayartsev updated this revision to Diff 13942.Sep 22 2014, 11:12 AM
ayartsev retitled this revision from to unique_ptr with ability to get a raw pointer after release..
ayartsev updated this object.
ayartsev edited the test plan for this revision. (Show Details)
ayartsev added a reviewer: dblaikie.
ayartsev added a subscriber: Unknown Object (MLST).
dblaikie edited edge metadata.Sep 22 2014, 12:41 PM

I'm really not sure this is the right approach... it's a bit creepy having
a std::unique_ptr own-yet-not-own like that. It also doesn't compose with
other deleters, which is a little unfortunate.

I'm wondering if this might be better approached by solving the more
general problem of "conditional ownership" which has turned up in several
places in LLVM and Clang. (ofter a T* and a boolean as members, the boolean
indicating whether the T* is owning or non-owning)

But to help demonstrate the use of this API - could you include the example
usage in this patch? (you mention a couple of code reviews where this came
up & I can't quite swap all the context back in from there to remember
exactly how this applies)

ayartsev updated this revision to Diff 14054.Sep 24 2014, 3:06 PM
ayartsev edited edge metadata.

It also doesn't compose with other deleters, which is a little unfortunate.

All is needed to do to make an existing deleter work with unique_ptr_deferred_release is to inherit the deleter from a 'SwitchControlledDeleterBase' class ('Switch' class in the new patch).

I'm wondering if this might be better approached by solving the more
general problem of "conditional ownership" which has turned up in several
places in LLVM and Clang. (ofter a T* and a boolean as members, the boolean
indicating whether the T* is owning or non-owning)

In the current implementation I tried to realize the same idea with T* managed by an unique_ptr descendant and a boolean wrapped by the 'Switch' class. Now I tend towards a clearer and safer approach with T* and a boolean member. Just working on the new patch.

But to help demonstrate the use of this API - could you include the example
usage in this patch? (you mention a couple of code reviews where this came
up & I can't quite swap all the context back in from there to remember
exactly how this applies)

Added refactored TGParser.cpp to the patch. Please look at TGParser::ParseDef(), TGParser::InstantiateMulticlassDef() and TGParser::ParseSimpleValue().

dblaikie added inline comments.Sep 24 2014, 3:36 PM
include/llvm/ADT/STLExtras.h
572

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).

lib/TableGen/TGParser.cpp
235

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)); ?)

The discussion moved to llvmdev, topic "New type of smart pointer for LLVM"

include/llvm/ADT/STLExtras.h
572

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.

lib/TableGen/TGParser.cpp
235

Yes, just a general cleanup, unique_ptr is sufficient here.