[ADT] Delete RefCountedBaseVPTR.

[jlebar]

Summary

This class is unnecessary.

Its comment indicated that it was a compile error to allocate an
instance of a class that inherits from RefCountedBaseVPTR on the stack.
This may have been true at one point, but it's not today.

Moreover you really do not want to allocate *any* refcounted object on
the stack, vptrs or not, so if we did have a way to prevent these
objects from being stack-allocated, we'd want to apply it to regular
RefCountedBase too, obviating the need for a separate RefCountedBaseVPTR
class.

It seems that the main way RefCountedBaseVPTR provides safety is by
making its subclass's destructor virtual. This may have been helpful at
one point, but these days clang will emit an error if you define a class
with virtual functions that inherits from RefCountedBase but doesn't
have a virtual destructor.

[dblaikie]

Looks good - thanks for the cleanup!

llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp

14–37 ↗

(On Diff #82681)

I'm not sure we even need test coverage for this anymore.

If IntrusiveRefCntPtr calls any dtor (& we should have some coverage of that, to be sure) then it'd be basically impossible for it to call the dtor in a way that wouldn't be compatible with that dtor dispatching if it was virtual. (ie: this test sort of boils down to testing virtual dispatch of virtual dtors - a feature of the compiler, not of this library)

But up to you.

[jlebar]

llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp

14–37 ↗

(On Diff #82681)

sgtm. I got rid of the virtual test and moved the explicit checking for destructor calls into the non-virtual test below. Will land the patch with that change.

cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h

class VariantMatcher {
 
   private:
     ast_type_traits::ASTNodeKind NodeKind;
   };
 
   /// \brief Payload interface to be specialized by each matcher type.
   ///
   /// It follows a similar interface as VariantMatcher itself.
-  class Payload : public RefCountedBaseVPTR {
+  class Payload : public RefCountedBase<Payload> {
   public:
-    ~Payload() override;
+    virtual ~Payload();
     virtual llvm::Optional<DynTypedMatcher> getSingleMatcher() const = 0;
     virtual std::string getTypeAsString() const = 0;
     virtual llvm::Optional<DynTypedMatcher>
     getTypedMatcher(const MatcherOps &Ops) const = 0;
     virtual bool isConvertibleTo(ast_type_traits::ASTNodeKind Kind,
                                  unsigned *Specificity) const = 0;
   };
 

cfe/trunk/include/clang/Basic/LLVM.h

namespace llvm {
 
   template<typename T>
   struct SaveAndRestore;
 
   // Reference counting.
   template <typename T> class IntrusiveRefCntPtr;
   template <typename T> struct IntrusiveRefCntPtrInfo;
   template <class Derived> class RefCountedBase;
-  class RefCountedBaseVPTR;
 
   class raw_ostream;
   class raw_pwrite_stream;
   // TODO: DenseMap, ...
 }
 
 
 namespace clang {
namespace clang {
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
   using llvm::SaveAndRestore;
 
   // Reference counting.
   using llvm::IntrusiveRefCntPtr;
   using llvm::IntrusiveRefCntPtrInfo;
   using llvm::RefCountedBase;
-  using llvm::RefCountedBaseVPTR;
 
   using llvm::raw_ostream;
   using llvm::raw_pwrite_stream;
 } // end namespace clang.
 
 #endif

cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h

void Profile(llvm::FoldingSetNodeID &ID) const {
     End.Profile(ID);
   }
 };
 
 //===----------------------------------------------------------------------===//
 // Path "pieces" for path-sensitive diagnostics.
 //===----------------------------------------------------------------------===//
 
-class PathDiagnosticPiece : public RefCountedBaseVPTR {
+class PathDiagnosticPiece : public RefCountedBase<PathDiagnosticPiece> {
 public:
   enum Kind { ControlFlow, Event, Macro, Call, Note };
   enum DisplayHint { Above, Below };
 
 private:
   const std::string str;
   const Kind kind;
   const DisplayHint Hint;
private:
   void operator=(const PathDiagnosticPiece &P) = delete;
 
 protected:
   PathDiagnosticPiece(StringRef s, Kind k, DisplayHint hint = Below);
 
   PathDiagnosticPiece(Kind k, DisplayHint hint = Below);
 
 public:
-  ~PathDiagnosticPiece() override;
+  virtual ~PathDiagnosticPiece();
 
   StringRef getString() const { return str; }
 
   /// Tag this PathDiagnosticPiece with the given C-string.
   void setTag(const char *tag) { Tag = tag; }
   
   /// Return the opaque tag (if any) on the PathDiagnosticPiece.
   const void *getTag() const { return Tag.data(); }

llvm/trunk/include/llvm/ADT/IntrusiveRefCntPtr.h

 //== llvm/ADT/IntrusiveRefCntPtr.h - Smart Refcounting Pointer ---*- C++ -*-==//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines IntrusiveRefCntPtr, a template class that
 // implements a "smart" pointer for objects that maintain their own
-// internal reference count, and RefCountedBase/RefCountedBaseVPTR, two
-// generic base classes for objects that wish to have their lifetimes
-// managed using reference counting.
+// internal reference count, and RefCountedBase, a generic base class
+// for objects that wish to have their lifetimes managed using reference
+// counting.
 //
 // IntrusiveRefCntPtr is similar to Boost's intrusive_ptr with added
 // LLVM-style casting.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ADT_INTRUSIVEREFCNTPTR_H
 #define LLVM_ADT_INTRUSIVEREFCNTPTR_H
public:
 
     void Retain() const { ++ref_cnt; }
     void Release() const {
       assert (ref_cnt > 0 && "Reference count is already zero.");
       if (--ref_cnt == 0) delete static_cast<const Derived*>(this);
     }
   };
 
-//===----------------------------------------------------------------------===//
-/// RefCountedBaseVPTR - A class that has the same function as
-///  RefCountedBase, but with a virtual destructor. Should be used
-///  instead of RefCountedBase for classes that already have virtual
-///  methods to enforce dynamic allocation via 'new'. Classes that
-///  inherit from RefCountedBaseVPTR can't be allocated on stack -
-///  attempting to do this will produce a compile error.
-//===----------------------------------------------------------------------===//
-  class RefCountedBaseVPTR {
-    mutable unsigned ref_cnt = 0;
-
-    virtual void anchor();
-
-  protected:
-    RefCountedBaseVPTR() = default;
-    RefCountedBaseVPTR(const RefCountedBaseVPTR &) : ref_cnt(0) {}
-
-    virtual ~RefCountedBaseVPTR() = default;
-
-    void Retain() const { ++ref_cnt; }
-    void Release() const {
-      assert (ref_cnt > 0 && "Reference count is already zero.");
-      if (--ref_cnt == 0) delete this;
-    }
-
-    template <typename T>
-    friend struct IntrusiveRefCntPtrInfo;
-  };
-
-
   template <typename T> struct IntrusiveRefCntPtrInfo {
     static void retain(T *obj) { obj->Retain(); }
     static void release(T *obj) { obj->Release(); }
   };
 
 /// \brief A thread-safe version of \c llvm::RefCountedBase.
 ///
 /// A generic base class for objects that wish to have their lifetimes managed

 ///  IntrusivePtrAddRef/IntrusivePtrRelease.  The smart pointers
 ///  manage reference counts via the RAII idiom: upon creation of
 ///  smart pointer the reference count of the wrapped object is
 ///  incremented and upon destruction of the smart pointer the
 ///  reference count is decremented.  This class also safely handles
 ///  wrapping NULL pointers.
 ///
 /// Reference counting is implemented via calls to
-///  Obj->Retain()/Obj->Release(). Release() is required to destroy
-///  the object when the reference count reaches zero. Inheriting from
-///  RefCountedBase/RefCountedBaseVPTR takes care of this
-///  automatically.
+///  Obj->Retain()/Obj->Release(). Release() is required to destroy the
+///  object when the reference count reaches zero. Inheriting from
+///  RefCountedBase takes care of this automatically.
 //===----------------------------------------------------------------------===//
   template <typename T>
   class IntrusiveRefCntPtr {
     T* Obj = nullptr;
 
   public:
     typedef T element_type;
 

llvm/trunk/lib/Support/CMakeLists.txt

add_llvm_library(LLVMSupport
   FoldingSet.cpp
   FormattedStream.cpp
   FormatVariadic.cpp
   GlobPattern.cpp
   GraphWriter.cpp
   Hashing.cpp
   IntEqClasses.cpp
   IntervalMap.cpp
-  IntrusiveRefCntPtr.cpp
   JamCRC.cpp
   LEB128.cpp
   LineIterator.cpp
   Locale.cpp
   LockFileManager.cpp
   ManagedStatic.cpp
   MathExtras.cpp
   MemoryBuffer.cpp

llvm/trunk/lib/Support/IntrusiveRefCntPtr.cpp

-//== IntrusiveRefCntPtr.cpp - Smart Refcounting Pointer ----------*- C++ -*-==//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#include "llvm/ADT/IntrusiveRefCntPtr.h"
-
-using namespace llvm;
-
-void RefCountedBaseVPTR::anchor() { }

llvm/trunk/unittests/ADT/IntrusiveRefCntPtrTest.cpp

 //===- unittest/ADT/IntrusiveRefCntPtrTest.cpp ----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "gtest/gtest.h"
 
-namespace {
-struct VirtualRefCounted : public llvm::RefCountedBaseVPTR {
-  virtual void f() {}
-};
-}
-
 namespace llvm {
 
-// Run this test with valgrind to detect memory leaks.
-TEST(IntrusiveRefCntPtr, RefCountedBaseVPTRCopyDoesNotLeak) {
-  VirtualRefCounted *V1 = new VirtualRefCounted;
-  IntrusiveRefCntPtr<VirtualRefCounted> R1 = V1;
-  VirtualRefCounted *V2 = new VirtualRefCounted(*V1);
-  IntrusiveRefCntPtr<VirtualRefCounted> R2 = V2;
-}
+namespace {
+struct SimpleRefCounted : public RefCountedBase<SimpleRefCounted> {
+  SimpleRefCounted() { ++NumInstances; }
+  SimpleRefCounted(const SimpleRefCounted &) { ++NumInstances; }
+  ~SimpleRefCounted() { --NumInstances; }
 
-struct SimpleRefCounted : public RefCountedBase<SimpleRefCounted> {};
+  static int NumInstances;
+};
+int SimpleRefCounted::NumInstances = 0;
+} // anonymous namespace
 
-// Run this test with valgrind to detect memory leaks.
 TEST(IntrusiveRefCntPtr, RefCountedBaseCopyDoesNotLeak) {
+  EXPECT_EQ(0, SimpleRefCounted::NumInstances);
+  {
     SimpleRefCounted *S1 = new SimpleRefCounted;
     IntrusiveRefCntPtr<SimpleRefCounted> R1 = S1;
     SimpleRefCounted *S2 = new SimpleRefCounted(*S1);
     IntrusiveRefCntPtr<SimpleRefCounted> R2 = S2;
+    EXPECT_EQ(2, SimpleRefCounted::NumInstances);
+  }
+  EXPECT_EQ(0, SimpleRefCounted::NumInstances);
 }
 
 struct InterceptRefCounted : public RefCountedBase<InterceptRefCounted> {
   InterceptRefCounted(bool *Released, bool *Retained)
     : Released(Released), Retained(Retained) {}
   bool * const Released;
   bool * const Retained;
 };