diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index 4c9c9a6bd66..86c765df521 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -105,8 +105,8 @@ grpc_arg ClientChannelNode::CreateChannelArg() { RefCountedPtr ClientChannelNode::MakeClientChannelNode( grpc_channel* channel, size_t channel_tracer_max_nodes, bool is_top_level_channel) { - return MakePolymorphicRefCounted( - channel, channel_tracer_max_nodes, is_top_level_channel); + return MakeRefCounted(channel, channel_tracer_max_nodes, + is_top_level_channel); } } // namespace channelz diff --git a/src/core/lib/gprpp/orphanable.h b/src/core/lib/gprpp/orphanable.h index d0ec9b6461d..3123e3f5a39 100644 --- a/src/core/lib/gprpp/orphanable.h +++ b/src/core/lib/gprpp/orphanable.h @@ -86,7 +86,8 @@ class InternallyRefCounted : public Orphanable { GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE // Allow RefCountedPtr<> to access Unref() and IncrementRefCount(). - friend class RefCountedPtr; + template + friend class RefCountedPtr; InternallyRefCounted() { gpr_ref_init(&refs_, 1); } virtual ~InternallyRefCounted() {} @@ -129,7 +130,8 @@ class InternallyRefCountedWithTracing : public Orphanable { GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE // Allow RefCountedPtr<> to access Unref() and IncrementRefCount(). - friend class RefCountedPtr; + template + friend class RefCountedPtr; InternallyRefCountedWithTracing() : InternallyRefCountedWithTracing(static_cast(nullptr)) {} diff --git a/src/core/lib/gprpp/ref_counted.h b/src/core/lib/gprpp/ref_counted.h index ddac5bd4755..03c293f6ed9 100644 --- a/src/core/lib/gprpp/ref_counted.h +++ b/src/core/lib/gprpp/ref_counted.h @@ -73,7 +73,8 @@ class RefCounted { private: // Allow RefCountedPtr<> to access IncrementRefCount(). - friend class RefCountedPtr; + template + friend class RefCountedPtr; void IncrementRefCount() { gpr_ref(&refs_); } @@ -152,7 +153,8 @@ class RefCountedWithTracing { private: // Allow RefCountedPtr<> to access IncrementRefCount(). - friend class RefCountedPtr; + template + friend class RefCountedPtr; void IncrementRefCount() { gpr_ref(&refs_); } diff --git a/src/core/lib/gprpp/ref_counted_ptr.h b/src/core/lib/gprpp/ref_counted_ptr.h index 534d3d03cb4..c2dfbdd90f0 100644 --- a/src/core/lib/gprpp/ref_counted_ptr.h +++ b/src/core/lib/gprpp/ref_counted_ptr.h @@ -36,25 +36,49 @@ class RefCountedPtr { RefCountedPtr(std::nullptr_t) {} // If value is non-null, we take ownership of a ref to it. - explicit RefCountedPtr(T* value) { value_ = value; } + template + explicit RefCountedPtr(Y* value) { + value_ = value; + } - // Move support. + // Move ctors. RefCountedPtr(RefCountedPtr&& other) { value_ = other.value_; other.value_ = nullptr; } + template + RefCountedPtr(RefCountedPtr&& other) { + value_ = other.value_; + other.value_ = nullptr; + } + + // Move assignment. RefCountedPtr& operator=(RefCountedPtr&& other) { if (value_ != nullptr) value_->Unref(); value_ = other.value_; other.value_ = nullptr; return *this; } + template + RefCountedPtr& operator=(RefCountedPtr&& other) { + if (value_ != nullptr) value_->Unref(); + value_ = other.value_; + other.value_ = nullptr; + return *this; + } - // Copy support. + // Copy ctors. RefCountedPtr(const RefCountedPtr& other) { if (other.value_ != nullptr) other.value_->IncrementRefCount(); value_ = other.value_; } + template + RefCountedPtr(const RefCountedPtr& other) { + if (other.value_ != nullptr) other.value_->IncrementRefCount(); + value_ = other.value_; + } + + // Copy assignment. RefCountedPtr& operator=(const RefCountedPtr& other) { // Note: Order of reffing and unreffing is important here in case value_ // and other.value_ are the same object. @@ -63,17 +87,32 @@ class RefCountedPtr { value_ = other.value_; return *this; } + template + RefCountedPtr& operator=(const RefCountedPtr& other) { + // 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_->IncrementRefCount(); + if (value_ != nullptr) value_->Unref(); + value_ = other.value_; + return *this; + } ~RefCountedPtr() { if (value_ != nullptr) value_->Unref(); } // If value is non-null, we take ownership of a ref to it. - void reset(T* value = nullptr) { + template + void reset(Y* value) { if (value_ != nullptr) value_->Unref(); value_ = value; } + void reset() { + if (value_ != nullptr) value_->Unref(); + value_ = nullptr; + } + // TODO(roth): This method exists solely as a transition mechanism to allow // us to pass a ref to idiomatic C code that does not use RefCountedPtr<>. // Once all of our code has been converted to idiomatic C++, this @@ -89,16 +128,34 @@ class RefCountedPtr { T& operator*() const { return *value_; } T* operator->() const { return value_; } - bool operator==(const RefCountedPtr& other) const { + template + bool operator==(const RefCountedPtr& other) const { return value_ == other.value_; } - bool operator==(const T* other) const { return value_ == other; } - bool operator!=(const RefCountedPtr& other) const { + + template + bool operator==(const Y* other) const { + return value_ == other; + } + + bool operator==(std::nullptr_t) const { return value_ == nullptr; } + + template + bool operator!=(const RefCountedPtr& other) const { return value_ != other.value_; } - bool operator!=(const T* other) const { return value_ != other; } + + template + bool operator!=(const Y* other) const { + return value_ != other; + } + + bool operator!=(std::nullptr_t) const { return value_ != nullptr; } private: + template + friend class RefCountedPtr; + T* value_ = nullptr; }; @@ -107,11 +164,6 @@ inline RefCountedPtr MakeRefCounted(Args&&... args) { return RefCountedPtr(New(std::forward(args)...)); } -template -inline RefCountedPtr MakePolymorphicRefCounted(Args&&... args) { - return RefCountedPtr(New(std::forward(args)...)); -} - } // namespace grpc_core #endif /* GRPC_CORE_LIB_GPRPP_REF_COUNTED_PTR_H */ diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc index 99d9a4847fd..f224457a557 100644 --- a/test/core/channel/channel_trace_test.cc +++ b/test/core/channel/channel_trace_test.cc @@ -187,8 +187,8 @@ TEST_P(ChannelTracerTest, ComplexTest) { AddSimpleTrace(&tracer); AddSimpleTrace(&tracer); AddSimpleTrace(&tracer); - sc1.reset(nullptr); - sc2.reset(nullptr); + sc1.reset(); + sc2.reset(); } // Test a case in which the parent channel has subchannels and the subchannels @@ -234,9 +234,9 @@ TEST_P(ChannelTracerTest, TestNesting) { grpc_slice_from_static_string("subchannel one inactive"), sc1); AddSimpleTrace(&tracer); ValidateChannelTrace(&tracer, 8, GetParam()); - sc1.reset(nullptr); - sc2.reset(nullptr); - conn1.reset(nullptr); + sc1.reset(); + sc2.reset(); + conn1.reset(); } INSTANTIATE_TEST_CASE_P(ChannelTracerTestSweep, ChannelTracerTest, diff --git a/test/core/gprpp/ref_counted_ptr_test.cc b/test/core/gprpp/ref_counted_ptr_test.cc index aa30b72282f..463b5e89667 100644 --- a/test/core/gprpp/ref_counted_ptr_test.cc +++ b/test/core/gprpp/ref_counted_ptr_test.cc @@ -127,7 +127,7 @@ TEST(RefCountedPtr, ResetFromNonNullToNull) { TEST(RefCountedPtr, ResetFromNullToNull) { RefCountedPtr foo; EXPECT_EQ(nullptr, foo.get()); - foo.reset(nullptr); + foo.reset(); EXPECT_EQ(nullptr, foo.get()); } @@ -175,6 +175,67 @@ TEST(RefCountedPtr, RefCountedWithTracing) { foo->Unref(DEBUG_LOCATION, "foo"); } +class BaseClass : public RefCounted { + public: + BaseClass() {} +}; + +class Subclass : public BaseClass { + public: + Subclass() {} +}; + +TEST(RefCountedPtr, ConstructFromSubclass) { + RefCountedPtr p(New()); +} + +TEST(RefCountedPtr, CopyAssignFromSubclass) { + RefCountedPtr b; + EXPECT_EQ(nullptr, b.get()); + RefCountedPtr s = MakeRefCounted(); + b = s; + EXPECT_NE(nullptr, b.get()); +} + +TEST(RefCountedPtr, MoveAssignFromSubclass) { + RefCountedPtr b; + EXPECT_EQ(nullptr, b.get()); + RefCountedPtr s = MakeRefCounted(); + b = std::move(s); + EXPECT_NE(nullptr, b.get()); +} + +TEST(RefCountedPtr, ResetFromSubclass) { + RefCountedPtr b; + EXPECT_EQ(nullptr, b.get()); + b.reset(New()); + EXPECT_NE(nullptr, b.get()); +} + +TEST(RefCountedPtr, EqualityWithSubclass) { + Subclass* s = New(); + RefCountedPtr b(s); + EXPECT_EQ(b, s); +} + +void FunctionTakingBaseClass(RefCountedPtr p) { + p.reset(); // To appease clang-tidy. +} + +TEST(RefCountedPtr, CanPassSubclassToFunctionExpectingBaseClass) { + RefCountedPtr p = MakeRefCounted(); + FunctionTakingBaseClass(p); +} + +void FunctionTakingSubclass(RefCountedPtr p) { + p.reset(); // To appease clang-tidy. +} + +TEST(RefCountedPtr, CanPassSubclassToFunctionExpectingSubclass) { + RefCountedPtr p = MakeRefCounted(); + FunctionTakingSubclass(p); +} + } // namespace } // namespace testing } // namespace grpc_core