unique_ptr with ability to get a raw pointer after release.

[ayartsev]

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!

[dblaikie]

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]

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]

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

[ayartsev]

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.

include/llvm/ADT/STLExtras.h

 #endif
 
 struct FreeDeleter {
   void operator()(void* v) {
     ::free(v);
   }
 };
 
+class Switch {
+  bool TurnedOn;
+public:
+  Switch() : TurnedOn(true) {}
+  void TurnOn() { TurnedOn = true; }
+  void TurnOff() { TurnedOn = false; }
+  bool IsTurnedOn() const { return TurnedOn; }
+};
+
+template <class T>
+class SwitchControlledDeleter : public Switch {
+public:
+  void operator()(T* p) {
+    if (IsTurnedOn())
+      delete(p);
+  }
+};
+
+/// \brief unique_ptr that allows accessing a raw pointer after release.
+/// 
+/// Intended for use in cases when conditional releases are mixed with raw
+/// pointer access.
+/// 
+/// Example:
+///
+///   unique_ptr_deferred_release<Type> P(new P);
+///   if (cond) {
+///     P.release();
+///   }
+///   assert(P.get()); // 'true' regardless of whether release() was called 
+///                    // or not.
+///   NonOwningFunction(P.get());
+template<class T, class Deleter = SwitchControlledDeleter<T>> 
+class unique_ptr_deferred_release : public std::unique_ptr<T, Deleter> {
+public:
+  typedef typename std::unique_ptr<T, Deleter> Base;
+
+  unique_ptr_deferred_release() {}
+  explicit unique_ptr_deferred_release(T* p) : Base(p) {};
+
+  typename Base::pointer release() {
+    Base::get_deleter().TurnOff();
+    return Base::get();
+  }
+
+  void reset(pointer p = pointer()) {
+    if (p != Base::get()) {
+      Base::reset(p);
+      Base::get_deleter().TurnOn();
+    }
+  }
+
+  bool released() const {
+    return !Base::get_deleter().IsTurnedOn();
+  }
+};
+
 template<typename First, typename Second>
 struct pair_hash {
   size_t operator()(const std::pair<First, Second> &P) const {
     return std::hash<First>()(P.first) * 31 + std::hash<Second>()(P.second);
   }
 };
 
 } // End llvm namespace
 
 #endif

lib/TableGen/TGParser.cpp

bool TGParser::AddSubMultiClass(MultiClass *CurMC,
   int newDefStart = CurMC->DefPrototypes.size();
 
   // Add all of the defs in the subclass into the current multiclass.
   for (MultiClass::RecordVector::const_iterator i = SMC->DefPrototypes.begin(),
          iend = SMC->DefPrototypes.end();
        i != iend;
        ++i) {
     // Clone the def and add it to the current multiclass
-    Record *NewDef = new Record(**i);
+    auto NewDef = make_unique<Record>(**i);
 
     // Add all of the values in the superclass into the current def.
     for (unsigned i = 0, e = MCVals.size(); i != e; ++i)
-      if (AddValue(NewDef, SubMultiClass.RefRange.Start, MCVals[i])) {
-        delete NewDef;
+      if (AddValue(NewDef.get(), SubMultiClass.RefRange.Start, MCVals[i]))
         return true;
-      }
 
-    CurMC->DefPrototypes.push_back(NewDef);
+    CurMC->DefPrototypes.push_back(NewDef.release());
   }
 
   const std::vector<Init *> &SMCTArgs = SMC->Rec.getTemplateArgs();
 
   // Ensure that an appropriate number of template arguments are
   // specified.
   if (SMCTArgs.size() < SubMultiClass.TemplateArgs.size())
     return Error(SubMultiClass.RefRange.Start,
for (int64_t i = 0; i < List->getSize(); ++i) {
       IterVals.pop_back();
     }
     return false;
   }
 
   // This is the bottom of the recursion. We have all of the iterator values
   // for this point in the iteration space.  Instantiate a new record to
   // reflect this combination of values.
-  Record *IterRec = new Record(*CurRec);
+  unique_ptr_deferred_release<Record> IterRec(new Record(*CurRec));
 
   // Set the iterator values now.
   for (unsigned i = 0, e = IterVals.size(); i != e; ++i) {
     VarInit *IterVar = IterVals[i].IterVar;
     TypedInit *IVal = dyn_cast<TypedInit>(IterVals[i].IterValue);
     if (!IVal) {
       Error(Loc, "foreach iterator value is untyped");
-      delete IterRec;
       return true;
     }
 
     IterRec->addValue(RecordVal(IterVar->getName(), IVal->getType(), false));
 
-    if (SetValue(IterRec, Loc, IterVar->getName(),
+    if (SetValue(IterRec.get(), Loc, IterVar->getName(),
                  std::vector<unsigned>(), IVal)) {
       Error(Loc, "when instantiating this def");
-      delete IterRec;
       return true;
     }
 
     // Resolve it next.
     IterRec->resolveReferencesTo(IterRec->getValue(IterVar->getName()));
 
     // Remove it.
     IterRec->removeValue(IterVar->getName());
   }
 
   if (Records.getDef(IterRec->getNameInitAsString())) {
     // If this record is anonymous, it's no problem, just generate a new name
     if (IterRec->isAnonymous())
       IterRec->setName(GetNewAnonymousName());
     else {
       Error(Loc, "def already exists: " + IterRec->getNameInitAsString());
-      delete IterRec;
       return true;
     }
   }
 
-  Records.addDef(IterRec);
+  Records.addDef(IterRec.release());
   IterRec->resolveReferences();
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // Parser Code
 //===----------------------------------------------------------------------===//
 
case tgtok::Id: {
     if (Lex.getCode() != tgtok::greater) {
       TokError("expected '>' at end of value list");
       return nullptr;
     }
     Lex.Lex();  // eat the '>'
     SMLoc EndLoc = Lex.getLoc();
 
     // Create the new record, set it as CurRec temporarily.
-    Record *NewRec = new Record(GetNewAnonymousName(), NameLoc, Records,
-                                /*IsAnonymous=*/true);
+    unique_ptr_deferred_release<Record> 
+        NewRec(new Record(GetNewAnonymousName(), NameLoc, Records,
+                          /*IsAnonymous=*/true));
     SubClassReference SCRef;
     SCRef.RefRange = SMRange(NameLoc, EndLoc);
     SCRef.Rec = Class;
     SCRef.TemplateArgs = ValueList;
     // Add info about the subclass to NewRec.
-    if (AddSubClass(NewRec, SCRef)) {
-      delete NewRec;
+    if (AddSubClass(NewRec.get(), SCRef))
       return nullptr;
-    }
     if (!CurMultiClass) {
       NewRec->resolveReferences();
-      Records.addDef(NewRec);
+      Records.addDef(NewRec.release());
     } else {
       // This needs to get resolved once the multiclass template arguments are
       // known before any use.
       NewRec->setResolveFirst(true);
       // Otherwise, we're inside a multiclass, add it to the multiclass.
-      CurMultiClass->DefPrototypes.push_back(NewRec);
+      CurMultiClass->DefPrototypes.push_back(NewRec.release());
 
       // Copy the template arguments for the multiclass into the def.
       const std::vector<Init *> &TArgs =
                                   CurMultiClass->Rec.getTemplateArgs();
 
       for (unsigned i = 0, e = TArgs.size(); i != e; ++i) {
         const RecordVal *RV = CurMultiClass->Rec.getValue(TArgs[i]);
         assert(RV && "Template arg doesn't exist?");
if (!CurMultiClass) {
                                           VarInit::get(MCNameRV->getName(),
                                                        MCNameRV->getType()),
                                           NewRec->getNameInit(),
                                           StringRecTy::get()),
                            Class->getDefInit()->getType());
     }
 
     // The result of the expression is a reference to the new record.
-    return DefInit::get(NewRec);
+    return DefInit::get(NewRec.get());
   }
   case tgtok::l_brace: {           // Value ::= '{' ValueList '}'
     SMLoc BraceLoc = Lex.getLoc();
     Lex.Lex(); // eat the '{'
     std::vector<Init*> Vals;
 
     if (Lex.getCode() != tgtok::r_brace) {
       Vals = ParseValueList(CurRec);

 ///   DefInst ::= DEF ObjectName ObjectBody
 ///
 bool TGParser::ParseDef(MultiClass *CurMultiClass) {
   SMLoc DefLoc = Lex.getLoc();
   assert(Lex.getCode() == tgtok::Def && "Unknown tok");
   Lex.Lex();  // Eat the 'def' token.
 
   // Parse ObjectName and make a record for it.
-  Record *CurRec;
-  bool CurRecOwnershipTransferred = false;
+  unique_ptr_deferred_release<Record> CurRec;
+
   Init *Name = ParseObjectName(CurMultiClass);
   if (Name)
-    CurRec = new Record(Name, DefLoc, Records);
+    CurRec.reset(new Record(Name, DefLoc, Records));
   else
-    CurRec = new Record(GetNewAnonymousName(), DefLoc, Records,
-                        /*IsAnonymous=*/true);
+    CurRec.reset(new Record(GetNewAnonymousName(), DefLoc, Records,
+                            /*IsAnonymous=*/true));
 
   if (!CurMultiClass && Loops.empty()) {
     // Top-level def definition.
 
     // Ensure redefinition doesn't happen.
     if (Records.getDef(CurRec->getNameInitAsString())) {
       Error(DefLoc, "def '" + CurRec->getNameInitAsString()
             + "' already defined");
-      delete CurRec;
       return true;
     }
-    Records.addDef(CurRec);
-    CurRecOwnershipTransferred = true;
 
-    if (ParseObjectBody(CurRec))
+    Records.addDef(CurRec.release());
+
+    if (ParseObjectBody(CurRec.get()))
       return true;
   } else if (CurMultiClass) {
     // Parse the body before adding this prototype to the DefPrototypes vector.
     // That way implicit definitions will be added to the DefPrototypes vector
     // before this object, instantiated prior to defs derived from this object,
     // and this available for indirect name resolution when defs derived from
     // this object are instantiated.
-    if (ParseObjectBody(CurRec)) {
-      delete CurRec;
+    if (ParseObjectBody(CurRec.get()))
       return true;
-    }
 
     // Otherwise, a def inside a multiclass, add it to the multiclass.
     for (unsigned i = 0, e = CurMultiClass->DefPrototypes.size(); i != e; ++i)
       if (CurMultiClass->DefPrototypes[i]->getNameInit()
           == CurRec->getNameInit()) {
         Error(DefLoc, "def '" + CurRec->getNameInitAsString() +
               "' already defined in this multiclass!");
-        delete CurRec;
         return true;
       }
-    CurMultiClass->DefPrototypes.push_back(CurRec);
-    CurRecOwnershipTransferred = true;
-  } else if (ParseObjectBody(CurRec)) {
-    delete CurRec;
+    CurMultiClass->DefPrototypes.push_back(CurRec.release());
+  } else if (ParseObjectBody(CurRec.get())) {
     return true;
   }
 
   if (!CurMultiClass)  // Def's in multiclasses aren't really defs.
     // See Record::setName().  This resolve step will see any new name
     // for the def that might have been created when resolving
     // inheritance, values and arguments above.
     CurRec->resolveReferences();
 
   // If ObjectBody has template arguments, it's an error.
   assert(CurRec->getTemplateArgs().empty() && "How'd this get template args?");
 
   if (CurMultiClass) {
     // Copy the template arguments for the multiclass into the def.
     const std::vector<Init *> &TArgs =
                                 CurMultiClass->Rec.getTemplateArgs();
 
     for (unsigned i = 0, e = TArgs.size(); i != e; ++i) {
       const RecordVal *RV = CurMultiClass->Rec.getValue(TArgs[i]);
       assert(RV && "Template arg doesn't exist?");
       CurRec->addValue(*RV);
     }
   }
 
-  if (ProcessForeachDefs(CurRec, DefLoc)) {
+  if (ProcessForeachDefs(CurRec.get(), DefLoc)) {
     Error(DefLoc,
           "Could not process loops for def" + CurRec->getNameInitAsString());
-    if (!CurRecOwnershipTransferred)
-      delete CurRec;
     return true;
   }
 
-  if (!CurRecOwnershipTransferred)
-    delete CurRec;
   return false;
 }
 
 /// ParseForeach - Parse a for statement.  Return the record corresponding
 /// to it.  This returns true on error.
 ///
 ///   Foreach ::= FOREACH Declaration IN '{ ObjectList '}'
 ///   Foreach ::= FOREACH Declaration IN Object
DefName =
                      UnOpInit::get(UnOpInit::CAST, DefmPrefix,
                                    StringRecTy::get())->Fold(DefProto, &MC),
                      DefName, StringRecTy::get())->Fold(DefProto, &MC);
   }
 
   // Make a trail of SMLocs from the multiclass instantiations.
   SmallVector<SMLoc, 4> Locs(1, DefmPrefixRange.Start);
   Locs.append(DefProto->getLoc().begin(), DefProto->getLoc().end());
-  Record *CurRec = new Record(DefName, Locs, Records, IsAnonymous);
+  unique_ptr_deferred_release<Record> 
+      CurRec(new Record(DefName, Locs, Records, IsAnonymous));
 
   SubClassReference Ref;
   Ref.RefRange = DefmPrefixRange;
   Ref.Rec = DefProto;
-  AddSubClass(CurRec, Ref);
+  AddSubClass(CurRec.get(), Ref);
 
   // Set the value for NAME. We don't resolve references to it 'til later,
   // though, so that uses in nested multiclass names don't get
   // confused.
-  if (SetValue(CurRec, Ref.RefRange.Start, "NAME", std::vector<unsigned>(),
-               DefmPrefix)) {
+  if (SetValue(CurRec.get(), Ref.RefRange.Start, "NAME", 
+               std::vector<unsigned>(), DefmPrefix)) {
     Error(DefmPrefixRange.Start, "Could not resolve "
           + CurRec->getNameInitAsString() + ":NAME to '"
           + DefmPrefix->getAsUnquotedString() + "'");
-    delete CurRec;
     return nullptr;
   }
 
   // If the DefNameString didn't resolve, we probably have a reference to
   // NAME and need to replace it. We need to do at least this much greedily,
   // otherwise nested multiclasses will end up with incorrect NAME expansions.
   if (!DefNameString) {
     RecordVal *DefNameRV = CurRec->getValue("NAME");
if (!CurMultiClass) {
     // defs as more than one probably refers to NAME or some other
     // common internal placeholder.
 
     // Ensure redefinition doesn't happen.
     if (Records.getDef(CurRec->getNameInitAsString())) {
       Error(DefmPrefixRange.Start, "def '" + CurRec->getNameInitAsString() +
             "' already defined, instantiating defm with subdef '" + 
             DefProto->getNameInitAsString() + "'");
-      delete CurRec;
       return nullptr;
     }
 
-    Records.addDef(CurRec);
+    Records.addDef(CurRec.release());
   }
 
-  return CurRec;
+  return CurRec.release();
 }
 
 bool TGParser::ResolveMulticlassDefArgs(MultiClass &MC,
                                         Record *CurRec,
                                         SMLoc DefmPrefixLoc,
                                         SMLoc SubClassLoc,
                                         const std::vector<Init *> &TArgs,
                                         std::vector<Init *> &TemplateVals,

unittests/ADT/CMakeLists.txt

set(ADTSources
   SparseBitVectorTest.cpp
   SparseMultiSetTest.cpp
   SparseSetTest.cpp
   StringMapTest.cpp
   StringRefTest.cpp
   TinyPtrVectorTest.cpp
   TripleTest.cpp
   TwineTest.cpp
+  UniquePtrDeferredReleaseTest.cpp
   VariadicFunctionTest.cpp
  )
 
 add_llvm_unittest(ADTTests
   ${ADTSources}
   )

unittests/ADT/UniquePtrDeferredReleaseTest.cpp

+//===- UniquePtrDeferredReleaseTest.cpp -----------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace llvm;
+
+namespace {
+
+TEST(UniquePtrDeferredRelease, Release) {
+  unique_ptr_deferred_release<int> P(new int);
+  EXPECT_TRUE(P);
+
+  P.release();
+  EXPECT_TRUE(P);
+  EXPECT_TRUE(P.released());
+  EXPECT_TRUE(P.get());
+
+  delete P.get();
+}
+
+TEST(UniquePtrDeferredRelease, Reset) {
+  unique_ptr_deferred_release<int> P(new int);
+
+  P.reset();
+  EXPECT_FALSE(P);
+  EXPECT_FALSE(P.released());
+  EXPECT_FALSE(P.get());
+
+  P.release();
+  EXPECT_FALSE(P);
+  EXPECT_TRUE(P.released());
+  EXPECT_FALSE(P.get());
+
+  P.reset(new int);
+  EXPECT_TRUE(P);
+  EXPECT_FALSE(P.released());
+  EXPECT_TRUE(P.get());
+}
+
+TEST(UniquePtrDeferredRelease, Swap) {
+  unique_ptr_deferred_release<int> P1(new int);
+  unique_ptr_deferred_release<int> P2(new int);
+  P1.release();
+  P1.swap(P2);
+  EXPECT_TRUE(P1);
+  EXPECT_FALSE(P1.released());
+  EXPECT_TRUE(P1.get());
+  EXPECT_TRUE(P2);
+  EXPECT_TRUE(P2.released());
+  EXPECT_TRUE(P2.get());
+
+  delete P2.get();
+}
+} // end anonymous namespace