From ba916b7bd4e27cb93cad1e2bbefe337f2d38ae42 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 12 Nov 2019 10:24:43 +0100 Subject: [PATCH] Mark scoped_refptr move and swap operations as noexcept to align with chromium scoped_refptr implementation and prefer move over copy in some cases. Bug: webrtc:11078 Change-Id: I3178e74e611e4b23435668878e6bcc98bc2ce77d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/159541 Commit-Queue: Danil Chapovalov Reviewed-by: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#29768} --- api/BUILD.gn | 2 + api/scoped_refptr.h | 12 ++-- api/scoped_refptr_unittest.cc | 111 ++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 6 deletions(-) create mode 100644 api/scoped_refptr_unittest.cc diff --git a/api/BUILD.gn b/api/BUILD.gn index 2ebf6e6c63..1c6c952acf 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -954,6 +954,7 @@ if (rtc_include_tests) { "rtp_packet_info_unittest.cc", "rtp_packet_infos_unittest.cc", "rtp_parameters_unittest.cc", + "scoped_refptr_unittest.cc", "test/loopback_media_transport_unittest.cc", ] @@ -966,6 +967,7 @@ if (rtc_include_tests) { ":rtc_event_log_output_file", ":rtp_packet_info", ":rtp_parameters", + ":scoped_refptr", "../rtc_base:checks", "../rtc_base:gunit_helpers", "../rtc_base:rtc_base_approved", diff --git a/api/scoped_refptr.h b/api/scoped_refptr.h index 67d179fe7d..fa4e83dbaf 100644 --- a/api/scoped_refptr.h +++ b/api/scoped_refptr.h @@ -92,10 +92,10 @@ class scoped_refptr { } // Move constructors. - scoped_refptr(scoped_refptr&& r) : ptr_(r.release()) {} + scoped_refptr(scoped_refptr&& r) noexcept : ptr_(r.release()) {} template - scoped_refptr(scoped_refptr&& r) : ptr_(r.release()) {} + scoped_refptr(scoped_refptr&& r) noexcept : ptr_(r.release()) {} ~scoped_refptr() { if (ptr_) @@ -136,24 +136,24 @@ class scoped_refptr { return *this = r.get(); } - scoped_refptr& operator=(scoped_refptr&& r) { + scoped_refptr& operator=(scoped_refptr&& r) noexcept { scoped_refptr(std::move(r)).swap(*this); return *this; } template - scoped_refptr& operator=(scoped_refptr&& r) { + scoped_refptr& operator=(scoped_refptr&& r) noexcept { scoped_refptr(std::move(r)).swap(*this); return *this; } - void swap(T** pp) { + void swap(T** pp) noexcept { T* p = ptr_; ptr_ = *pp; *pp = p; } - void swap(scoped_refptr& r) { swap(&r.ptr_); } + void swap(scoped_refptr& r) noexcept { swap(&r.ptr_); } protected: T* ptr_; diff --git a/api/scoped_refptr_unittest.cc b/api/scoped_refptr_unittest.cc new file mode 100644 index 0000000000..75a202bccd --- /dev/null +++ b/api/scoped_refptr_unittest.cc @@ -0,0 +1,111 @@ +/* + * Copyright 2019 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "api/scoped_refptr.h" + +#include +#include + +#include "test/gtest.h" + +namespace rtc { +namespace { + +struct FunctionsCalled { + int addref = 0; + int release = 0; +}; + +class ScopedRefCounted { + public: + explicit ScopedRefCounted(FunctionsCalled* called) : called_(*called) {} + ScopedRefCounted(const ScopedRefCounted&) = delete; + ScopedRefCounted& operator=(const ScopedRefCounted&) = delete; + + void AddRef() { + ++called_.addref; + ++ref_count_; + } + void Release() { + ++called_.release; + if (0 == --ref_count_) + delete this; + } + + private: + ~ScopedRefCounted() = default; + + FunctionsCalled& called_; + int ref_count_ = 0; +}; + +TEST(ScopedRefptrTest, IsCopyConstructable) { + FunctionsCalled called; + scoped_refptr ptr = new ScopedRefCounted(&called); + scoped_refptr another_ptr = ptr; + + EXPECT_TRUE(ptr); + EXPECT_TRUE(another_ptr); + EXPECT_EQ(called.addref, 2); +} + +TEST(ScopedRefptrTest, IsCopyAssignable) { + FunctionsCalled called; + scoped_refptr another_ptr; + scoped_refptr ptr = new ScopedRefCounted(&called); + another_ptr = ptr; + + EXPECT_TRUE(ptr); + EXPECT_TRUE(another_ptr); + EXPECT_EQ(called.addref, 2); +} + +TEST(ScopedRefptrTest, IsMoveConstructableWithoutExtraAddRefRelease) { + FunctionsCalled called; + scoped_refptr ptr = new ScopedRefCounted(&called); + scoped_refptr another_ptr = std::move(ptr); + + EXPECT_FALSE(ptr); + EXPECT_TRUE(another_ptr); + EXPECT_EQ(called.addref, 1); + EXPECT_EQ(called.release, 0); +} + +TEST(ScopedRefptrTest, IsMoveAssignableWithoutExtraAddRefRelease) { + FunctionsCalled called; + scoped_refptr another_ptr; + scoped_refptr ptr = new ScopedRefCounted(&called); + another_ptr = std::move(ptr); + + EXPECT_FALSE(ptr); + EXPECT_TRUE(another_ptr); + EXPECT_EQ(called.addref, 1); + EXPECT_EQ(called.release, 0); +} + +TEST(ScopedRefptrTest, MovableDuringVectorReallocation) { + static_assert( + std::is_nothrow_move_constructible>(), + ""); + // Test below describes a scenario where it is helpful for move constructor + // to be noexcept. + FunctionsCalled called; + std::vector> ptrs; + ptrs.reserve(1); + // Insert more elements than reserved to provoke reallocation. + ptrs.push_back(new ScopedRefCounted(&called)); + ptrs.push_back(new ScopedRefCounted(&called)); + + EXPECT_EQ(called.addref, 2); + EXPECT_EQ(called.release, 0); +} + +} // namespace +} // namespace rtc