Merge pull request #20475 from veblush/memory-cleanup

Clean up memory code

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc

@@ -151,6 +151,7 @@ class GrpcLb : public LoadBalancingPolicy {
    public:
     explicit BalancerCallState(
         RefCountedPtr<LoadBalancingPolicy> parent_grpclb_policy);
+    ~BalancerCallState();
 
     // It's the caller's responsibility to ensure that Orphan() is called from
     // inside the combiner.
@@ -164,10 +165,6 @@ class GrpcLb : public LoadBalancingPolicy {
     bool seen_serverlist() const { return seen_serverlist_; }
 
    private:
-    GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
-    ~BalancerCallState();
-
     GrpcLb* grpclb_policy() const {
       return static_cast<GrpcLb*>(grpclb_policy_.get());
     }

src/core/ext/filters/client_channel/resolver.h

@@ -87,6 +87,7 @@ class Resolver : public InternallyRefCounted<Resolver> {
   // Not copyable nor movable.
   Resolver(const Resolver&) = delete;
   Resolver& operator=(const Resolver&) = delete;
+  virtual ~Resolver();
 
   /// Starts resolving.
   virtual void StartLocked() = 0;
@@ -121,8 +122,6 @@ class Resolver : public InternallyRefCounted<Resolver> {
   }
 
  protected:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
   /// Does NOT take ownership of the reference to \a combiner.
   // TODO(roth): Once we have a C++-like interface for combiners, this
   // API should change to take a RefCountedPtr<>, so that we always take
@@ -130,8 +129,6 @@ class Resolver : public InternallyRefCounted<Resolver> {
   explicit Resolver(grpc_combiner* combiner,
                     UniquePtr<ResultHandler> result_handler);
 
-  virtual ~Resolver();
-
   /// Shuts down the resolver.
   virtual void ShutdownLocked() = 0;
 

src/core/ext/filters/client_channel/retry_throttle.h

@@ -32,6 +32,7 @@ class ServerRetryThrottleData : public RefCounted<ServerRetryThrottleData> {
  public:
   ServerRetryThrottleData(intptr_t max_milli_tokens, intptr_t milli_token_ratio,
                           ServerRetryThrottleData* old_throttle_data);
+  ~ServerRetryThrottleData();
 
   /// Records a failure.  Returns true if it's okay to send a retry.
   bool RecordFailure();
@@ -43,10 +44,6 @@ class ServerRetryThrottleData : public RefCounted<ServerRetryThrottleData> {
   intptr_t milli_token_ratio() const { return milli_token_ratio_; }
 
  private:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
-  ~ServerRetryThrottleData();
-
   void GetReplacementThrottleDataIfNeeded(
       ServerRetryThrottleData** throttle_data);
 

src/core/lib/gprpp/README.md

@@ -9,8 +9,4 @@ the use of portability macros.
 
 Note that this is the only place in src/core where we allow
 use of the C++ standard library (i.e., anything in the `std::`
-namespace).  And for now, we require that any use of the
-standard library is build-time-only -- i.e., we do not allow
-run-time dependencies on libstdc++.  For more details, see
-[gRFC L6](/grpc/proposal/blob/master/L6-allow-c%2B%2B-in-grpc-core.md) and
-[Moving gRPC core to C++](/grpc/grpc/blob/master/doc/core/moving-to-c%2B%2B.md).
+namespace). 

src/core/lib/gprpp/memory.h

@@ -28,24 +28,6 @@
 #include <memory>
 #include <utility>
 
-// Add this to a class that want to use Delete(), but has a private or
-// protected destructor.
-// Should not be used in new code.
-// TODO(juanlishen): Remove this macro, and instead comment that the public dtor
-// should not be used directly.
-#define GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE \
-  template <typename _Delete_T>                   \
-  friend void ::grpc_core::Delete(_Delete_T*);
-
-// Add this to a class that want to use New(), but has a private or
-// protected constructor.
-// Should not be used in new code.
-// TODO(juanlishen): Remove this macro, and instead comment that the public dtor
-// should not be used directly.
-#define GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW      \
-  template <typename _New_T, typename... _New_Args> \
-  friend _New_T* grpc_core::New(_New_Args&&...);
-
 namespace grpc_core {
 
 // Alternative to new, to ensure memory allocation being wrapped to gpr_malloc

src/core/lib/gprpp/orphanable.h

@@ -81,8 +81,6 @@ class InternallyRefCounted : public Orphanable {
   InternallyRefCounted& operator=(const InternallyRefCounted&) = delete;
 
  protected:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
   // Allow RefCountedPtr<> to access Unref() and IncrementRefCount().
   template <typename T>
   friend class RefCountedPtr;

src/core/lib/gprpp/ref_counted.h

@@ -39,9 +39,7 @@ namespace grpc_core {
 
 // PolymorphicRefCount enforces polymorphic destruction of RefCounted.
 class PolymorphicRefCount {
- protected:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
+ public:
   virtual ~PolymorphicRefCount() = default;
 };
 
@@ -49,9 +47,7 @@ class PolymorphicRefCount {
 // RefCounted. Please refer to grpc_core::RefCounted for more details, and
 // when in doubt use PolymorphicRefCount.
 class NonPolymorphicRefCount {
- protected:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
+ public:
   ~NonPolymorphicRefCount() = default;
 };
 
@@ -230,6 +226,9 @@ class RefCount {
 template <typename Child, typename Impl = PolymorphicRefCount>
 class RefCounted : public Impl {
  public:
+  // Note: Depending on the Impl used, this dtor can be implicitly virtual.
+  ~RefCounted() = default;
+
   RefCountedPtr<Child> Ref() GRPC_MUST_USE_RESULT {
     IncrementRefCount();
     return RefCountedPtr<Child>(static_cast<Child*>(this));
@@ -266,8 +265,6 @@ class RefCounted : public Impl {
   RefCounted& operator=(const RefCounted&) = delete;
 
  protected:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
   // TraceFlagT is defined to accept both DebugOnlyTraceFlag and TraceFlag.
   // Note: RefCount tracing is only enabled on debug builds, even when a
   //       TraceFlag is used.
@@ -276,9 +273,6 @@ class RefCounted : public Impl {
                       intptr_t initial_refcount = 1)
       : refs_(initial_refcount, trace_flag) {}
 
-  // Note: Depending on the Impl used, this dtor can be implicitly virtual.
-  ~RefCounted() = default;
-
  private:
   // Allow RefCountedPtr<> to access IncrementRefCount().
   template <typename T>

src/core/lib/iomgr/buffer_list.h

@@ -114,6 +114,10 @@ struct Timestamps {
 #ifdef GRPC_LINUX_ERRQUEUE
 class TracedBuffer {
  public:
+  /** Use AddNewEntry function instead of using this directly. */
+  TracedBuffer(uint32_t seq_no, void* arg)
+      : seq_no_(seq_no), arg_(arg), next_(nullptr) {}
+
   /** Add a new entry in the TracedBuffer list pointed to by head. Also saves
    * sendmsg_time with the current timestamp. */
   static void AddNewEntry(grpc_core::TracedBuffer** head, uint32_t seq_no,
@@ -133,11 +137,6 @@ class TracedBuffer {
                        grpc_error* shutdown_err);
 
  private:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
-
-  TracedBuffer(uint32_t seq_no, void* arg)
-      : seq_no_(seq_no), arg_(arg), next_(nullptr) {}
-
   uint32_t seq_no_; /* The sequence number for the last byte in the buffer */
   void* arg_;       /* The arg to pass to timestamps_callback */
   grpc_core::Timestamps ts_; /* The timestamps corresponding to this buffer */

src/core/lib/iomgr/cfstream_handle.h

@@ -43,10 +43,12 @@ class CFStreamHandle : public GrpcLibraryInitHolder {
  public:
   static CFStreamHandle* CreateStreamHandle(CFReadStreamRef read_stream,
                                             CFWriteStreamRef write_stream);
-  ~CFStreamHandle() override;
+  /** Use CreateStreamHandle function instead of using this directly. */
+  CFStreamHandle(CFReadStreamRef read_stream, CFWriteStreamRef write_stream);
   CFStreamHandle(const CFStreamHandle& ref) = delete;
   CFStreamHandle(CFStreamHandle&& ref) = delete;
   CFStreamHandle& operator=(const CFStreamHandle& rhs) = delete;
+  ~CFStreamHandle() override;
 
   void NotifyOnOpen(grpc_closure* closure);
   void NotifyOnRead(grpc_closure* closure);
@@ -57,7 +59,6 @@ class CFStreamHandle : public GrpcLibraryInitHolder {
   void Unref(const char* file = "", int line = 0, const char* reason = nullptr);
 
  private:
-  CFStreamHandle(CFReadStreamRef read_stream, CFWriteStreamRef write_stream);
   static void ReadCallback(CFReadStreamRef stream, CFStreamEventType type,
                            void* client_callback_info);
   static void WriteCallback(CFWriteStreamRef stream, CFStreamEventType type,
@@ -72,9 +73,6 @@ class CFStreamHandle : public GrpcLibraryInitHolder {
   dispatch_queue_t dispatch_queue_;
 
   gpr_refcount refcount_;
-
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
 };
 
 #ifdef DEBUG

src/core/lib/slice/slice.cc

@@ -82,14 +82,11 @@ class NewSliceRefcount {
               &base_),
         user_destroy_(destroy),
         user_data_(user_data) {}
-
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
+  ~NewSliceRefcount() { user_destroy_(user_data_); }
 
   grpc_slice_refcount* base_refcount() { return &base_; }
 
  private:
-  ~NewSliceRefcount() { user_destroy_(user_data_); }
-
   grpc_slice_refcount base_;
   RefCount refs_;
   void (*user_destroy_)(void*);
@@ -147,14 +144,11 @@ class NewWithLenSliceRefcount {
         user_data_(user_data),
         user_length_(user_length),
         user_destroy_(destroy) {}
-
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
+  ~NewWithLenSliceRefcount() { user_destroy_(user_data_, user_length_); }
 
   grpc_slice_refcount* base_refcount() { return &base_; }
 
  private:
-  ~NewWithLenSliceRefcount() { user_destroy_(user_data_, user_length_); }
-
   grpc_slice_refcount base_;
   RefCount refs_;
   void* user_data_;
@@ -170,8 +164,6 @@ class MovedStringSliceRefCount {
               &base_),
         str_(std::move(str)) {}
 
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
   grpc_slice_refcount* base_refcount() { return &base_; }
 
  private:

src/core/lib/slice/slice_hash_table.h

@@ -65,6 +65,10 @@ class SliceHashTable : public RefCounted<SliceHashTable<T>> {
                                               Entry* entries,
                                               ValueCmp value_cmp);
 
+  // Use Create function instead of using this directly.
+  SliceHashTable(size_t num_entries, Entry* entries, ValueCmp value_cmp);
+  virtual ~SliceHashTable();
+
   /// Returns the value from the table associated with \a key.
   /// Returns null if \a key is not found.
   const T* Get(const grpc_slice& key) const;
@@ -78,12 +82,6 @@ class SliceHashTable : public RefCounted<SliceHashTable<T>> {
   static int Cmp(const SliceHashTable& a, const SliceHashTable& b);
 
  private:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
-
-  SliceHashTable(size_t num_entries, Entry* entries, ValueCmp value_cmp);
-  virtual ~SliceHashTable();
-
   void Add(const grpc_slice& key, T& value);
 
   // Default value comparison function, if none specified by caller.

src/core/lib/slice/slice_weak_hash_table.h

@@ -44,6 +44,10 @@ class SliceWeakHashTable : public RefCounted<SliceWeakHashTable<T, Size>> {
     return MakeRefCounted<SliceWeakHashTable<T, Size>>();
   }
 
+  /// Use Create function instead of using this directly.
+  SliceWeakHashTable() = default;
+  ~SliceWeakHashTable() = default;
+
   /// Add a mapping from \a key to \a value, taking ownership of \a key. This
   /// operation will always succeed. It may discard older entries.
   void Add(const grpc_slice& key, T value) {
@@ -61,12 +65,6 @@ class SliceWeakHashTable : public RefCounted<SliceWeakHashTable<T, Size>> {
   }
 
  private:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
-  SliceWeakHashTable() = default;
-  ~SliceWeakHashTable() = default;
-
   /// The type of the table "rows".
   class Entry {
    public:

src/core/tsi/ssl/session_cache/ssl_session_cache.h

@@ -53,6 +53,10 @@ class SslSessionLRUCache : public grpc_core::RefCounted<SslSessionLRUCache> {
     return grpc_core::MakeRefCounted<SslSessionLRUCache>(capacity);
   }
 
+  // Use Create function instead of using this directly.
+  explicit SslSessionLRUCache(size_t capacity);
+  ~SslSessionLRUCache();
+
   // Not copyable nor movable.
   SslSessionLRUCache(const SslSessionLRUCache&) = delete;
   SslSessionLRUCache& operator=(const SslSessionLRUCache&) = delete;
@@ -67,14 +71,8 @@ class SslSessionLRUCache : public grpc_core::RefCounted<SslSessionLRUCache> {
   SslSessionPtr Get(const char* key);
 
  private:
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
-  GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
   class Node;
 
-  explicit SslSessionLRUCache(size_t capacity);
-  ~SslSessionLRUCache();
-
   Node* FindLocked(const grpc_slice& key);
   void Remove(Node* node);
   void PushFront(Node* node);