Code review changes.

BUILD

@@ -545,7 +545,6 @@ grpc_cc_library(
 
 grpc_cc_library(
     name = "reference_counted",
-    srcs = ["src/core/lib/support/reference_counted.cc"],
     public_hdrs = ["src/core/lib/support/reference_counted.h"],
     language = "c++",
     deps = [

CMakeLists.txt

@@ -809,7 +809,6 @@ add_library(gpr
   src/core/lib/support/log_windows.cc
   src/core/lib/support/mpscq.cc
   src/core/lib/support/murmur_hash.cc
-  src/core/lib/support/reference_counted.cc
   src/core/lib/support/stack_lockfree.cc
   src/core/lib/support/string.cc
   src/core/lib/support/string_posix.cc

Makefile

@@ -2846,7 +2846,6 @@ LIBGPR_SRC = \
     src/core/lib/support/log_windows.cc \
     src/core/lib/support/mpscq.cc \
     src/core/lib/support/murmur_hash.cc \
-    src/core/lib/support/reference_counted.cc \
     src/core/lib/support/stack_lockfree.cc \
     src/core/lib/support/string.cc \
     src/core/lib/support/string_posix.cc \

build.yaml

@@ -49,7 +49,6 @@ filegroups:
   - src/core/lib/support/log_windows.cc
   - src/core/lib/support/mpscq.cc
   - src/core/lib/support/murmur_hash.cc
-  - src/core/lib/support/reference_counted.cc
   - src/core/lib/support/stack_lockfree.cc
   - src/core/lib/support/string.cc
   - src/core/lib/support/string_posix.cc

config.m4

@@ -62,7 +62,6 @@ if test "$PHP_GRPC" != "no"; then
     src/core/lib/support/log_windows.cc \
     src/core/lib/support/mpscq.cc \
     src/core/lib/support/murmur_hash.cc \
-    src/core/lib/support/reference_counted.cc \
     src/core/lib/support/stack_lockfree.cc \
     src/core/lib/support/string.cc \
     src/core/lib/support/string_posix.cc \

config.w32

@@ -39,7 +39,6 @@ if (PHP_GRPC != "no") {
     "src\\core\\lib\\support\\log_windows.cc " +
     "src\\core\\lib\\support\\mpscq.cc " +
     "src\\core\\lib\\support\\murmur_hash.cc " +
-    "src\\core\\lib\\support\\reference_counted.cc " +
     "src\\core\\lib\\support\\stack_lockfree.cc " +
     "src\\core\\lib\\support\\string.cc " +
     "src\\core\\lib\\support\\string_posix.cc " +

gRPC-Core.podspec

@@ -229,7 +229,6 @@ Pod::Spec.new do |s|
                       'src/core/lib/support/log_windows.cc',
                       'src/core/lib/support/mpscq.cc',
                       'src/core/lib/support/murmur_hash.cc',
-                      'src/core/lib/support/reference_counted.cc',
                       'src/core/lib/support/stack_lockfree.cc',
                       'src/core/lib/support/string.cc',
                       'src/core/lib/support/string_posix.cc',

grpc.gemspec

@@ -126,7 +126,6 @@ Gem::Specification.new do |s|
   s.files += %w( src/core/lib/support/log_windows.cc )
   s.files += %w( src/core/lib/support/mpscq.cc )
   s.files += %w( src/core/lib/support/murmur_hash.cc )
-  s.files += %w( src/core/lib/support/reference_counted.cc )
   s.files += %w( src/core/lib/support/stack_lockfree.cc )
   s.files += %w( src/core/lib/support/string.cc )
   s.files += %w( src/core/lib/support/string_posix.cc )

grpc.gyp

@@ -184,7 +184,6 @@
         'src/core/lib/support/log_windows.cc',
         'src/core/lib/support/mpscq.cc',
         'src/core/lib/support/murmur_hash.cc',
-        'src/core/lib/support/reference_counted.cc',
         'src/core/lib/support/stack_lockfree.cc',
         'src/core/lib/support/string.cc',
         'src/core/lib/support/string_posix.cc',

package.xml

@@ -138,7 +138,6 @@
     <file baseinstalldir="/" name="src/core/lib/support/log_windows.cc" role="src" />
     <file baseinstalldir="/" name="src/core/lib/support/mpscq.cc" role="src" />
     <file baseinstalldir="/" name="src/core/lib/support/murmur_hash.cc" role="src" />
-    <file baseinstalldir="/" name="src/core/lib/support/reference_counted.cc" role="src" />
     <file baseinstalldir="/" name="src/core/lib/support/stack_lockfree.cc" role="src" />
     <file baseinstalldir="/" name="src/core/lib/support/string.cc" role="src" />
     <file baseinstalldir="/" name="src/core/lib/support/string_posix.cc" role="src" />

src/core/lib/support/debug_location.h

@@ -21,6 +21,9 @@
 
 namespace grpc_core {
 
+// Used for tracking file and line where a call is made for debug builds.
+// No-op for non-debug builds.
+// Callers can use the DEBUG_LOCATION macro in either case.
 #ifndef NDEBUG
 class DebugLocation {
  public:

src/core/lib/support/reference_counted.cc

@@ -1,58 +0,0 @@
-/*
- *
- * Copyright 2017 gRPC authors.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- *
- */
-
-#include "src/core/lib/support/reference_counted.h"
-
-#include <grpc/support/log.h>
-
-#include "src/core/lib/support/memory.h"
-
-namespace grpc_core {
-
-void ReferenceCounted::Ref(const DebugLocation& location, const char* reason) {
-  if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) {
-    gpr_atm old_refs = gpr_atm_no_barrier_load(&refs_.count);
-    gpr_log(GPR_DEBUG, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s",
-            trace_flag_->name(), this, location.file(), location.line(),
-            old_refs, old_refs + 1, reason);
-  }
-  Ref();
-}
-
-void ReferenceCounted::Ref() { gpr_ref(&refs_); }
-
-bool ReferenceCounted::Unref(const DebugLocation& location,
-                             const char* reason) {
-  if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) {
-    gpr_atm old_refs = gpr_atm_no_barrier_load(&refs_.count);
-    gpr_log(GPR_DEBUG, "%s:%p %s:%d unref %" PRIdPTR " -> %" PRIdPTR " %s",
-            trace_flag_->name(), this, location.file(), location.line(),
-            old_refs, old_refs - 1, reason);
-  }
-  return Unref();
-}
-
-bool ReferenceCounted::Unref() {
-  if (gpr_unref(&refs_)) {
-    Delete(this);
-    return true;
-  }
-  return false;
-}
-
-}  // namespace grpc_core

src/core/lib/support/reference_counted.h

@@ -20,19 +20,46 @@
 #define GRPC_CORE_LIB_SUPPORT_REFERENCE_COUNTED_H
 
 #include <grpc/support/sync.h>
+#include <grpc/support/log.h>
 
 #include "src/core/lib/debug/trace.h"
 #include "src/core/lib/support/debug_location.h"
+#include "src/core/lib/support/memory.h"
 
 namespace grpc_core {
 
+// A base class for reference-counted objects.
+// New objects should be created via New() and start with a refcount of 1.
+// When the refcount reaches 0, the object will be deleted via Delete().
 class ReferenceCounted {
  public:
-  void Ref();
+  void Ref() { gpr_ref(&refs_); }
-  void Ref(const DebugLocation& location, const char* reason);
 
-  bool Unref();
+  void Ref(const DebugLocation& location, const char* reason) {
-  bool Unref(const DebugLocation& location, const char* reason);
+    if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) {
+      gpr_atm old_refs = gpr_atm_no_barrier_load(&refs_.count);
+      gpr_log(GPR_DEBUG, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s",
+              trace_flag_->name(), this, location.file(), location.line(),
+              old_refs, old_refs + 1, reason);
+    }
+    Ref();
+  }
+
+  void Unref() {
+    if (gpr_unref(&refs_)) {
+      Delete(this);
+    }
+  }
+
+  void Unref(const DebugLocation& location, const char* reason) {
+    if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) {
+      gpr_atm old_refs = gpr_atm_no_barrier_load(&refs_.count);
+      gpr_log(GPR_DEBUG, "%s:%p %s:%d unref %" PRIdPTR " -> %" PRIdPTR " %s",
+              trace_flag_->name(), this, location.file(), location.line(),
+              old_refs, old_refs - 1, reason);
+    }
+    Unref();
+  }
 
   // Not copyable nor movable.
   ReferenceCounted(const ReferenceCounted&) = delete;
@@ -43,6 +70,8 @@ class ReferenceCounted {
   template <typename T>
   friend void Delete(T*);
 
+  ReferenceCounted() : ReferenceCounted(nullptr) {}
+
   explicit ReferenceCounted(TraceFlag* trace_flag) : trace_flag_(trace_flag) {
     gpr_ref_init(&refs_, 1);
   }
@@ -50,7 +79,7 @@ class ReferenceCounted {
   virtual ~ReferenceCounted() {}
 
  private:
-  TraceFlag* trace_flag_;
+  TraceFlag* trace_flag_ = nullptr;
   gpr_refcount refs_;
 };
 

src/core/lib/support/reference_counted_ptr.h

@@ -53,8 +53,10 @@ class ReferenceCountedPtr {
     value_ = other.value_;
   }
   ReferenceCountedPtr& operator=(const ReferenceCountedPtr& other) {
-    if (value_ != nullptr) value_->Unref();
+    // Note: Order of reffing and unreffing is important here in case value_
+    // and other.value_ are the same object.
     if (other.value_ != nullptr) other.value_->Ref();
+    if (value_ != nullptr) value_->Unref();
     value_ = other.value_;
     return *this;
   }

src/python/grpcio/grpc_core_dependencies.py

@@ -38,7 +38,6 @@ CORE_SOURCE_FILES = [
   'src/core/lib/support/log_windows.cc',
   'src/core/lib/support/mpscq.cc',
   'src/core/lib/support/murmur_hash.cc',
-  'src/core/lib/support/reference_counted.cc',
   'src/core/lib/support/stack_lockfree.cc',
   'src/core/lib/support/string.cc',
   'src/core/lib/support/string_posix.cc',

test/core/support/reference_counted_ptr_test.cc

@@ -79,6 +79,19 @@ TEST(ReferenceCountedPtr, CopyAssignment) {
   EXPECT_EQ(foo.get(), foo2.get());
 }
 
+TEST(ReferenceCountedPtr, CopyAssignmentWhenEmpty) {
+  ReferenceCountedPtr<Foo> foo;
+  ReferenceCountedPtr<Foo> foo2;
+  foo2 = foo;
+  EXPECT_EQ(nullptr, foo.get());
+  EXPECT_EQ(nullptr, foo2.get());
+}
+
+TEST(ReferenceCountedPtr, CopyAssignmentToSelf) {
+  ReferenceCountedPtr<Foo> foo(New<Foo>());
+  foo = foo;
+}
+
 TEST(ReferenceCountedPtr, EnclosedScope) {
   ReferenceCountedPtr<Foo> foo(New<Foo>());
   {

test/core/support/reference_counted_test.cc

@@ -28,7 +28,7 @@ namespace testing {
 
 class Foo : public ReferenceCounted {
  public:
-  Foo() : ReferenceCounted(nullptr) {}
+  Foo() {}
 };
 
 TEST(ReferenceCounted, StackAllocated) { Foo foo; }

tools/doxygen/Doxyfile.core.internal

@@ -1299,7 +1299,6 @@ src/core/lib/support/mpscq.cc \
 src/core/lib/support/mpscq.h \
 src/core/lib/support/murmur_hash.cc \
 src/core/lib/support/murmur_hash.h \
-src/core/lib/support/reference_counted.cc \
 src/core/lib/support/reference_counted.h \
 src/core/lib/support/reference_counted_ptr.h \
 src/core/lib/support/spinlock.h \

tools/run_tests/generated/sources_and_headers.json

@@ -7808,7 +7808,6 @@
       "src/core/lib/support/log_windows.cc", 
       "src/core/lib/support/mpscq.cc", 
       "src/core/lib/support/murmur_hash.cc", 
-      "src/core/lib/support/reference_counted.cc", 
       "src/core/lib/support/stack_lockfree.cc", 
       "src/core/lib/support/string.cc", 
       "src/core/lib/support/string_posix.cc",