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

Антон Ярцев anton.yartsev at gmail.com
Wed Sep 24 19:15:03 PDT 2014


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.

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

http://reviews.llvm.org/D5443






More information about the llvm-commits mailing list