Code review changes.

19 files changed, 54 insertions, 78 deletions

diff --git a/BUILD b/BUILD
index 9c93a5c099..6ba5e6f655 100644
--- a/BUILD
+++ b/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 = [
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 7e18465c92..7b214be435 100644
--- a/CMakeLists.txt
+++ b/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
diff --git a/Makefile b/Makefile
index 848182d1bd..a81e62b440 100644
--- a/Makefile
+++ b/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 \
diff --git a/build.yaml b/build.yaml
index 9ac86eb430..57a65b3707 100644
--- a/build.yaml
+++ b/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
diff --git a/config.m4 b/config.m4
index 9d514c8552..d2f2520fea 100644
--- a/config.m4
+++ b/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 \
diff --git a/config.w32 b/config.w32
index 2f24fcd8b0..8a713751dc 100644
--- a/config.w32
+++ b/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 " +
diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec
index f623f26831..bc500cc0d7 100644
--- a/gRPC-Core.podspec
+++ b/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',
diff --git a/grpc.gemspec b/grpc.gemspec
index da1b6c0aef..d1e8b5254d 100644
--- a/grpc.gemspec
+++ b/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 )
diff --git a/grpc.gyp b/grpc.gyp
index 0ee6529013..fb153915ff 100644
--- a/grpc.gyp
+++ b/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',
diff --git a/package.xml b/package.xml
index 92eab40b87..6f786a13ad 100644
--- a/package.xml
+++ b/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" />
diff --git a/src/core/lib/support/debug_location.h b/src/core/lib/support/debug_location.h
index 47c5082874..0939da595d 100644
--- a/src/core/lib/support/debug_location.h
+++ b/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:
diff --git a/src/core/lib/support/reference_counted.cc b/src/core/lib/support/reference_counted.cc
deleted file mode 100644
index 222628a6fa..0000000000
--- a/src/core/lib/support/reference_counted.cc
+++ /dev/null
@@ -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
diff --git a/src/core/lib/support/reference_counted.h b/src/core/lib/support/reference_counted.h
index 42bedcbd3a..ff74de082c 100644
--- a/src/core/lib/support/reference_counted.h
+++ b/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(const DebugLocation& location, const char* reason);
+  void Ref() { gpr_ref(&refs_); }
 
-  bool Unref();
-  bool Unref(const DebugLocation& location, const char* reason);
+  void 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 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_;
 };
 
diff --git a/src/core/lib/support/reference_counted_ptr.h b/src/core/lib/support/reference_counted_ptr.h
index 3782380943..1590549bfb 100644
--- a/src/core/lib/support/reference_counted_ptr.h
+++ b/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;
   }
diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py
index 38f2a40133..330c4185c6 100644
--- a/src/python/grpcio/grpc_core_dependencies.py
+++ b/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',
diff --git a/test/core/support/reference_counted_ptr_test.cc b/test/core/support/reference_counted_ptr_test.cc
index 942c724724..14dd620887 100644
--- a/test/core/support/reference_counted_ptr_test.cc
+++ b/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>());
   {
diff --git a/test/core/support/reference_counted_test.cc b/test/core/support/reference_counted_test.cc
index c5795429c4..b1f79e61e6 100644
--- a/test/core/support/reference_counted_test.cc
+++ b/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; }
diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal
index 37ffff6b79..8343f88a37 100644
--- a/tools/doxygen/Doxyfile.core.internal
+++ b/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 \
diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json
index ed5128d9c1..284a2098e3 100644
--- a/tools/run_tests/generated/sources_and_headers.json
+++ b/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",