diff --git a/BUILD.bazel b/BUILD.bazel index 5882a034da..438c8ee206 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -316,7 +316,6 @@ objc_library( "src/butil/memory/aligned_memory.h", "src/butil/memory/scoped_policy.h", "src/butil/memory/scoped_ptr.h", - "src/butil/move.h", "src/butil/port.h", "src/butil/posix/eintr_wrapper.h", "src/butil/scoped_generic.h", diff --git a/docs/en/cpp14_move.md b/docs/en/cpp14_move.md new file mode 100644 index 0000000000..782d6172b1 --- /dev/null +++ b/docs/en/cpp14_move.md @@ -0,0 +1,39 @@ +# Migrating butil ownership transfers to C++14 + +The C++03 move emulation in `butil/move.h` has been replaced by native +rvalue references and `std::move` from ``. The header and +`MOVE_ONLY_TYPE_FOR_CPP_03` macro have been removed. + +For an existing butil resource owner, transfer ownership with `std::move`: + +```cpp +#include +#include "butil/memory/scoped_ptr.h" + +scoped_ptr source(new int(42)); +scoped_ptr destination(std::move(source)); +``` + +Return a local owner directly to allow copy elision or an implicit move. +`Pass()` and `scoped_ptr::PassAs()` have been removed. Replace ownership +transfers with `std::move(owner)`, or return the local owner directly, including +when converting a `scoped_ptr` to `scoped_ptr`. + +A discarded `owner.Pass()` used to release the resource through a temporary +owner. Replace such calls with the appropriate explicit cleanup operation +(`reset()`, `clear()`, or `Close()`). Discarding `std::move(owner)` does not +release anything. + +These types remain noncopyable. `ScopedGeneric` and `ScopedFILE` retain +their existing support for move construction only. `scoped_ptr`, +`ScopedVector`, and `File` also support move assignment. Custom deleters +and stateful traits retain their existing copying behavior. + +Downstream types using the removed macro must declare deleted copy +operations and implement native move operations that transfer ownership +and invalidate the source. Merely defaulting a move constructor for a raw +resource handle does not prevent both objects from releasing that handle. + +The exported `File` move constructor and move assignment signatures now +take `File&&`. Rebuild downstream binaries when updating the brpc shared +library; the old C++03-emulation symbols are no longer provided. diff --git a/docs/en/getting_started.md b/docs/en/getting_started.md index 2ac22c3a75..357c583794 100644 --- a/docs/en/getting_started.md +++ b/docs/en/getting_started.md @@ -389,6 +389,8 @@ Same with [here](#compile-brpc-with-cmake) c++14 is turned on by default to remove dependencies on boost (atomic). +For butil ownership transfers, see [Migrating to C++14 move semantics](cpp14_move.md). + The over-aligned issues in GCC7 is suppressed temporarily now. Using other versions of gcc may generate warnings, contact us to fix. diff --git a/src/bthread/execution_queue.cpp b/src/bthread/execution_queue.cpp index de4069b12a..241c962afb 100644 --- a/src/bthread/execution_queue.cpp +++ b/src/bthread/execution_queue.cpp @@ -344,14 +344,14 @@ ExecutionQueueBase::scoped_ptr_t ExecutionQueueBase::address(uint64_t id) { const uint32_t ver1 = _version_of_vref(vref1); if (ver1 == _version_of_id(id)) { ret.reset(m); - return ret.Pass(); + return ret; } const uint64_t vref2 = m->_versioned_ref.fetch_sub( 1, butil::memory_order_release); const int32_t nref = _ref_of_vref(vref2); if (nref > 1) { - return ret.Pass(); + return ret; } else if (__builtin_expect(nref == 1, 1)) { const uint32_t ver2 = _version_of_vref(vref2); if ((ver2 & 1)) { @@ -379,7 +379,7 @@ ExecutionQueueBase::scoped_ptr_t ExecutionQueueBase::address(uint64_t id) { CHECK(false) << "Over dereferenced id=" << id; } } - return ret.Pass(); + return ret; } int ExecutionQueueBase::create(uint64_t* id, const ExecutionQueueOptions* options, diff --git a/src/bthread/execution_queue_inl.h b/src/bthread/execution_queue_inl.h index 2a85d27beb..2e094a2da7 100644 --- a/src/bthread/execution_queue_inl.h +++ b/src/bthread/execution_queue_inl.h @@ -293,7 +293,7 @@ friend class TaskIterator; Base::scoped_ptr_t ptr = Base::address(id.value); Base* b = ptr.release(); scoped_ptr_t ret((self_type*)b); - return ret.Pass(); + return ret; } int execute(typename butil::add_const_reference::type task) { diff --git a/src/butil/containers/scoped_ptr_hash_map.h b/src/butil/containers/scoped_ptr_hash_map.h index d69b96a090..60a6cd7086 100644 --- a/src/butil/containers/scoped_ptr_hash_map.h +++ b/src/butil/containers/scoped_ptr_hash_map.h @@ -79,7 +79,7 @@ class ScopedPtrHashMap { scoped_ptr ret(it->second); it->second = nullptr; - return ret.Pass(); + return ret; } scoped_ptr take(const Key& k) { @@ -97,7 +97,7 @@ class ScopedPtrHashMap { scoped_ptr ret(it->second); data_.erase(it); - return ret.Pass(); + return ret; } scoped_ptr take_and_erase(const Key& k) { diff --git a/src/butil/environment.cc b/src/butil/environment.cc index 0978036d6a..0a2070ae84 100644 --- a/src/butil/environment.cc +++ b/src/butil/environment.cc @@ -229,7 +229,7 @@ scoped_ptr AlterEnvironment(const char* const* const env, result[i] = &storage_data[result_indices[i]]; result[result_indices.size()] = 0; // Null terminator. - return result.Pass(); + return result; } #endif // OS_POSIX diff --git a/src/butil/files/file.cc b/src/butil/files/file.cc index b3ce118ba2..138790bfc6 100644 --- a/src/butil/files/file.cc +++ b/src/butil/files/file.cc @@ -47,11 +47,11 @@ File::File(Error error_details) async_(false) { } -File::File(RValue other) - : file_(other.object->TakePlatformFile()), - error_details_(other.object->error_details()), - created_(other.object->created()), - async_(other.object->async_) { +File::File(File&& other) noexcept + : file_(other.TakePlatformFile()), + error_details_(other.error_details()), + created_(other.created()), + async_(other.async_) { } File::~File() { @@ -59,13 +59,13 @@ File::~File() { Close(); } -File& File::operator=(RValue other) { - if (this != other.object) { +File& File::operator=(File&& other) noexcept { + if (this != &other) { Close(); - SetPlatformFile(other.object->TakePlatformFile()); - error_details_ = other.object->error_details(); - created_ = other.object->created(); - async_ = other.object->async_; + SetPlatformFile(other.TakePlatformFile()); + error_details_ = other.error_details(); + created_ = other.created(); + async_ = other.async_; } return *this; } diff --git a/src/butil/files/file.h b/src/butil/files/file.h index 1b9ce175d9..4ba04c71de 100644 --- a/src/butil/files/file.h +++ b/src/butil/files/file.h @@ -19,7 +19,6 @@ #include "butil/base_export.h" #include "butil/basictypes.h" #include "butil/files/scoped_file.h" -#include "butil/move.h" #include "butil/time/time.h" #if defined(OS_WIN) @@ -53,8 +52,6 @@ typedef struct stat64 stat_wrapper_t; // to the OS is not considered const, even if there is no apparent change to // member variables. class BUTIL_EXPORT File { - MOVE_ONLY_TYPE_FOR_CPP_03(File, RValue) - public: // FLAG_(OPEN|CREATE).* are mutually exclusive. You should specify exactly one // of the five (possibly combining with other flags) when opening or creating @@ -167,13 +164,14 @@ class BUTIL_EXPORT File { // Creates an object with a specific error_details code. explicit File(Error error_details); - // Move constructor for C++03 move emulation of this type. - File(RValue other); + File(const File&) = delete; + File& operator=(const File&) = delete; + + File(File&& other) noexcept; ~File(); - // Move operator= for C++03 move emulation of this type. - File& operator=(RValue other); + File& operator=(File&& other) noexcept; // Creates or opens the given file. void Initialize(const FilePath& name, uint32_t flags); diff --git a/src/butil/files/memory_mapped_file.cc b/src/butil/files/memory_mapped_file.cc index 8f0e8f8d87..91365251bb 100644 --- a/src/butil/files/memory_mapped_file.cc +++ b/src/butil/files/memory_mapped_file.cc @@ -4,6 +4,8 @@ #include "butil/files/memory_mapped_file.h" +#include + #include "butil/files/file_path.h" #include "butil/logging.h" @@ -36,7 +38,7 @@ bool MemoryMappedFile::Initialize(File file) { if (IsValid()) return false; - file_ = file.Pass(); + file_ = std::move(file); if (!MapFileToMemory()) { CloseHandles(); diff --git a/src/butil/files/scoped_file.h b/src/butil/files/scoped_file.h index 1e6b93a729..83be347205 100644 --- a/src/butil/files/scoped_file.h +++ b/src/butil/files/scoped_file.h @@ -47,7 +47,6 @@ typedef ScopedGeneric ScopedFD; //// Automatically closes |FILE*|s. class ScopedFILE { - MOVE_ONLY_TYPE_FOR_CPP_03(ScopedFILE, RValue); public: ScopedFILE() : _fp(nullptr) {} @@ -61,10 +60,12 @@ class ScopedFILE { // destructor, otherwise the behavior is undefined explicit ScopedFILE(FILE *fp) :_fp(fp) {} - ScopedFILE(RValue rvalue) { - _fp = rvalue.object->_fp; - rvalue.object->_fp = nullptr; - } + ScopedFILE(const ScopedFILE&) = delete; + ScopedFILE& operator=(const ScopedFILE&) = delete; + + ScopedFILE(ScopedFILE&& other) noexcept : _fp(other.release()) {} + + ScopedFILE& operator=(ScopedFILE&&) = delete; ~ScopedFILE() { if (_fp != nullptr) { diff --git a/src/butil/memory/scoped_ptr.h b/src/butil/memory/scoped_ptr.h index f3356bf9d8..71f3f85ccf 100644 --- a/src/butil/memory/scoped_ptr.h +++ b/src/butil/memory/scoped_ptr.h @@ -33,56 +33,42 @@ // foo[10].Method(); // Foo::Method on the 10th element. // } // -// These scopers also implement part of the functionality of C++11 unique_ptr +// These scopers also implement part of the functionality of std::unique_ptr // in that they are "movable but not copyable." You can use the scopers in // the parameter and return types of functions to signify ownership transfer -// in to and out of a function. When calling a function that has a scoper -// as the argument type, it must be called with the result of an analogous -// scoper's Pass() function or another function that generates a temporary; -// passing by copy will NOT work. Here is an example using scoped_ptr: +// in to and out of a function. Use std::move() to transfer ownership from a +// named scoped_ptr; passing by copy will not work. Here is an example: // // void TakesOwnership(scoped_ptr arg) { // // Do something with arg // } // scoped_ptr CreateFoo() { -// // No need for calling Pass() because we are constructing a temporary -// // for the return value. // return scoped_ptr(new Foo("new")); // } // scoped_ptr PassThru(scoped_ptr arg) { -// return arg.Pass(); +// return arg; // } // // { // scoped_ptr ptr(new Foo("yay")); // ptr manages Foo("yay"). -// TakesOwnership(ptr.Pass()); // ptr no longer owns Foo("yay"). +// TakesOwnership(std::move(ptr)); // ptr no longer owns Foo("yay"). // scoped_ptr ptr2 = CreateFoo(); // ptr2 owns the return Foo. // scoped_ptr ptr3 = // ptr3 now owns what was in ptr2. -// PassThru(ptr2.Pass()); // ptr2 is correspondingly nullptr. +// PassThru(std::move(ptr2)); // ptr2 is correspondingly nullptr. // } // -// Notice that if you do not call Pass() when returning from PassThru(), or -// when invoking TakesOwnership(), the code will not compile because scopers -// are not copyable; they only implement move semantics which require calling -// the Pass() function to signify a destructive transfer of state. CreateFoo() -// is different though because we are constructing a temporary on the return -// line and thus can avoid needing to call Pass(). -// -// Pass() properly handles upcast in initialization, i.e. you can use a +// std::move() properly handles upcast in initialization, i.e. you can use a // scoped_ptr to initialize a scoped_ptr: // // scoped_ptr foo(new Foo()); -// scoped_ptr parent(foo.Pass()); +// scoped_ptr parent(std::move(foo)); // -// PassAs<>() should be used to upcast return value in return statement: +// A converting move constructor supports upcasting a local return value: // // scoped_ptr CreateFoo() { // scoped_ptr result(new FooChild()); -// return result.PassAs(); +// return result; // } -// -// Note that PassAs<>() is implemented only for scoped_ptr, but not for -// scoped_ptr. This is because casting array pointers may not be safe. #ifndef BUTIL_MEMORY_SCOPED_PTR_H_ #define BUTIL_MEMORY_SCOPED_PTR_H_ @@ -95,10 +81,11 @@ #include #include // For std::swap(). +#include +#include #include "butil/basictypes.h" #include "butil/compiler_specific.h" -#include "butil/move.h" #include "butil/type_traits.h" namespace butil { @@ -114,24 +101,11 @@ class RefCountedThreadSafeBase; template struct DefaultDeleter { DefaultDeleter() {} - template DefaultDeleter(const DefaultDeleter&) { - // IMPLEMENTATION NOTE: C++11 20.7.1.1.2p2 only provides this constructor - // if U* is implicitly convertible to T* and U is not an array type. - // - // Correct implementation should use SFINAE to disable this - // constructor. However, since there are no other 1-argument constructors, - // using a COMPILE_ASSERT() based on is_convertible<> and requiring - // complete types is simpler and will cause compile failures for equivalent - // misuses. - // - // Note, the is_convertible check also ensures that U is not an - // array. T is guaranteed to be a non-array, so any U* where U is an array - // cannot convert to T*. - enum { T_must_be_complete = sizeof(T) }; - enum { U_must_be_complete = sizeof(U) }; - COMPILE_ASSERT((butil::is_convertible::value), - U_ptr_must_implicitly_convert_to_T_ptr); - } + + template ::value, + int>::type = 0> + DefaultDeleter(const DefaultDeleter&) {} inline void operator()(T* ptr) const { enum { type_must_be_complete = sizeof(T) }; delete ptr; @@ -199,10 +173,7 @@ class scoped_ptr_impl { template scoped_ptr_impl(scoped_ptr_impl* other) : data_(other->release(), other->get_deleter()) { - // We do not support move-only deleters. We could modify our move - // emulation to have butil::subtle::move() and butil::subtle::forward() - // functions that are imperfect emulations of their C++11 equivalents, - // but until there's a requirement, just assume deleters are copyable. + // Deleters are copied intentionally. Move-only deleters are unsupported. } template @@ -301,14 +272,12 @@ class scoped_ptr_impl { // increase the size proportional to whatever state they need to have. See // comments inside scoped_ptr_impl<> for details. // -// Current implementation targets having a strict subset of C++11's +// Current implementation targets having a strict subset of C++14's // unique_ptr<> features. Known deficiencies include not supporting move-only -// deleteres, function pointers as deleters, and deleters with reference +// deleters, function pointers as deleters, and deleters with reference // types. template > class scoped_ptr { - MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue) - COMPILE_ASSERT(butil::internal::IsNotRefCounted::value, T_is_refcounted_type_and_needs_scoped_refptr); @@ -326,37 +295,35 @@ class scoped_ptr { // Constructor. Allows initialization of a stateful deleter. scoped_ptr(element_type* p, const D& d) : impl_(p, d) { } - // Constructor. Allows construction from a scoped_ptr rvalue for a - // convertible type and deleter. - // - // IMPLEMENTATION NOTE: C++11 unique_ptr<> keeps this constructor distinct - // from the normal move constructor. By C++11 20.7.1.2.1.21, this constructor - // has different post-conditions if D is a reference type. Since this - // implementation does not support deleters with reference type, - // we do not need a separate move constructor allowing us to avoid one - // use of SFINAE. You only need to care about this if you modify the - // implementation of scoped_ptr. - template - scoped_ptr(scoped_ptr other) : impl_(&other.impl_) { - COMPILE_ASSERT(!butil::is_array::value, U_cannot_be_an_array); - } + scoped_ptr(const scoped_ptr&) = delete; + scoped_ptr& operator=(const scoped_ptr&) = delete; - // Constructor. Move constructor for C++03 move emulation of this type. - scoped_ptr(RValue rvalue) : impl_(&rvalue.object->impl_) { } + // Move operations copy the deleter to preserve existing behavior. Move-only + // deleters are intentionally unsupported. + scoped_ptr(scoped_ptr&& other) : impl_(&other.impl_) { } - // operator=. Allows assignment from a scoped_ptr rvalue for a convertible - // type and deleter. - // - // IMPLEMENTATION NOTE: C++11 unique_ptr<> keeps this operator= distinct from - // the normal move assignment operator. By C++11 20.7.1.2.3.4, this templated - // form has different requirements on for move-only Deleters. Since this - // implementation does not support move-only Deleters, we do not need a - // separate move assignment operator allowing us to avoid one use of SFINAE. - // You only need to care about this if you modify the implementation of - // scoped_ptr. - template - scoped_ptr& operator=(scoped_ptr rhs) { - COMPILE_ASSERT(!butil::is_array::value, U_cannot_be_an_array); + scoped_ptr& operator=(scoped_ptr&& rhs) { + if (this != &rhs) { + impl_.TakeState(&rhs.impl_); + } + return *this; + } + + template ::value && + std::is_convertible::value && + std::is_constructible::value, + int>::type = 0> + scoped_ptr(scoped_ptr&& other) : impl_(&other.impl_) { } + + template ::value && + std::is_convertible::value && + std::is_assignable::value, + int>::type = 0> + scoped_ptr& operator=(scoped_ptr&& rhs) { impl_.TakeState(&rhs.impl_); return *this; } @@ -415,17 +382,6 @@ class scoped_ptr { return impl_.release(); } - // C++98 doesn't support functions templates with default parameters which - // makes it hard to write a PassAs() that understands converting the deleter - // while preserving simple calling semantics. - // - // Until there is a use case for PassAs() with custom deleters, just ignore - // the custom deleter. - template - scoped_ptr PassAs() { - return scoped_ptr(Pass()); - } - private: // Needed to reach into |impl_| in the constructor. template friend class scoped_ptr; @@ -444,8 +400,6 @@ class scoped_ptr { template class scoped_ptr { - MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue) - public: // The element and deleter types. typedef T element_type; @@ -472,12 +426,15 @@ class scoped_ptr { // NOT use implicit_cast() to upcast the static type of the array. explicit scoped_ptr(element_type* array) : impl_(array) { } - // Constructor. Move constructor for C++03 move emulation of this type. - scoped_ptr(RValue rvalue) : impl_(&rvalue.object->impl_) { } + scoped_ptr(const scoped_ptr&) = delete; + scoped_ptr& operator=(const scoped_ptr&) = delete; - // operator=. Move operator= for C++03 move emulation of this type. - scoped_ptr& operator=(RValue rhs) { - impl_.TakeState(&rhs.object->impl_); + scoped_ptr(scoped_ptr&& other) : impl_(&other.impl_) { } + + scoped_ptr& operator=(scoped_ptr&& rhs) { + if (this != &rhs) { + impl_.TakeState(&rhs.impl_); + } return *this; } diff --git a/src/butil/memory/scoped_vector.h b/src/butil/memory/scoped_vector.h index a2aeba703b..f97dff34ff 100644 --- a/src/butil/memory/scoped_vector.h +++ b/src/butil/memory/scoped_vector.h @@ -9,7 +9,6 @@ #include "butil/basictypes.h" #include "butil/logging.h" -#include "butil/move.h" #include "butil/stl_util.h" namespace butil { @@ -18,8 +17,6 @@ namespace butil { // destructor. template class ScopedVector { - MOVE_ONLY_TYPE_FOR_CPP_03(ScopedVector, RValue) - public: typedef typename std::vector::allocator_type allocator_type; typedef typename std::vector::size_type size_type; @@ -37,13 +34,16 @@ class ScopedVector { ScopedVector() {} ~ScopedVector() { clear(); } - ScopedVector(RValue other) { swap(*other.object); } -#if __cplusplus >= 201103L // >= C++11 + ScopedVector(const ScopedVector&) = delete; + ScopedVector& operator=(const ScopedVector&) = delete; + ScopedVector(ScopedVector&& other) noexcept { swap(other); } ScopedVector(std::initializer_list il) : v_(il) {} -#endif - ScopedVector& operator=(RValue rhs) { - swap(*rhs.object); + ScopedVector& operator=(ScopedVector&& rhs) { + if (this != &rhs) { + clear(); + swap(rhs); + } return *this; } diff --git a/src/butil/move.h b/src/butil/move.h deleted file mode 100644 index 6613d1c4e0..0000000000 --- a/src/butil/move.h +++ /dev/null @@ -1,218 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef BUTIL_MOVE_H_ -#define BUTIL_MOVE_H_ - -// Macro with the boilerplate that makes a type move-only in C++03. -// -// USAGE -// -// This macro should be used instead of DISALLOW_COPY_AND_ASSIGN to create -// a "move-only" type. Unlike DISALLOW_COPY_AND_ASSIGN, this macro should be -// the first line in a class declaration. -// -// A class using this macro must call .Pass() (or somehow be an r-value already) -// before it can be: -// -// * Passed as a function argument -// * Used as the right-hand side of an assignment -// * Returned from a function -// -// Each class will still need to define their own "move constructor" and "move -// operator=" to make this useful. Here's an example of the macro, the move -// constructor, and the move operator= from the scoped_ptr class: -// -// template -// class scoped_ptr { -// MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue) -// public: -// scoped_ptr(RValue& other) : ptr_(other.release()) { } -// scoped_ptr& operator=(RValue& other) { -// swap(other); -// return *this; -// } -// }; -// -// Note that the constructor must NOT be marked explicit. -// -// For consistency, the second parameter to the macro should always be RValue -// unless you have a strong reason to do otherwise. It is only exposed as a -// macro parameter so that the move constructor and move operator= don't look -// like they're using a phantom type. -// -// -// HOW THIS WORKS -// -// For a thorough explanation of this technique, see: -// -// http://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Move_Constructor -// -// The summary is that we take advantage of 2 properties: -// -// 1) non-const references will not bind to r-values. -// 2) C++ can apply one user-defined conversion when initializing a -// variable. -// -// The first lets us disable the copy constructor and assignment operator -// by declaring private version of them with a non-const reference parameter. -// -// For l-values, direct initialization still fails like in -// DISALLOW_COPY_AND_ASSIGN because the copy constructor and assignment -// operators are private. -// -// For r-values, the situation is different. The copy constructor and -// assignment operator are not viable due to (1), so we are trying to call -// a non-existent constructor and non-existing operator= rather than a private -// one. Since we have not committed an error quite yet, we can provide an -// alternate conversion sequence and a constructor. We add -// -// * a private struct named "RValue" -// * a user-defined conversion "operator RValue()" -// * a "move constructor" and "move operator=" that take the RValue& as -// their sole parameter. -// -// Only r-values will trigger this sequence and execute our "move constructor" -// or "move operator=." L-values will match the private copy constructor and -// operator= first giving a "private in this context" error. This combination -// gives us a move-only type. -// -// For signaling a destructive transfer of data from an l-value, we provide a -// method named Pass() which creates an r-value for the current instance -// triggering the move constructor or move operator=. -// -// Other ways to get r-values is to use the result of an expression like a -// function call. -// -// Here's an example with comments explaining what gets triggered where: -// -// class Foo { -// MOVE_ONLY_TYPE_FOR_CPP_03(Foo, RValue); -// -// public: -// ... API ... -// Foo(RValue other); // Move constructor. -// Foo& operator=(RValue rhs); // Move operator= -// }; -// -// Foo MakeFoo(); // Function that returns a Foo. -// -// Foo f; -// Foo f_copy(f); // ERROR: Foo(Foo&) is private in this context. -// Foo f_assign; -// f_assign = f; // ERROR: operator=(Foo&) is private in this context. -// -// -// Foo f(MakeFoo()); // R-value so alternate conversion executed. -// Foo f_copy(f.Pass()); // R-value so alternate conversion executed. -// f = f_copy.Pass(); // R-value so alternate conversion executed. -// -// -// IMPLEMENTATION SUBTLETIES WITH RValue -// -// The RValue struct is just a container for a pointer back to the original -// object. It should only ever be created as a temporary, and no external -// class should ever declare it or use it in a parameter. -// -// It is tempting to want to use the RValue type in function parameters, but -// excluding the limited usage here for the move constructor and move -// operator=, doing so would mean that the function could take both r-values -// and l-values equially which is unexpected. See COMPARED To Boost.Move for -// more details. -// -// An alternate, and incorrect, implementation of the RValue class used by -// Boost.Move makes RValue a fieldless child of the move-only type. RValue& -// is then used in place of RValue in the various operators. The RValue& is -// "created" by doing *reinterpret_cast(this). This has the appeal -// of never creating a temporary RValue struct even with optimizations -// disabled. Also, by virtue of inheritance you can treat the RValue -// reference as if it were the move-only type itself. Unfortunately, -// using the result of this reinterpret_cast<> is actually undefined behavior -// due to C++98 5.2.10.7. In certain compilers (e.g., NaCl) the optimizer -// will generate non-working code. -// -// In optimized builds, both implementations generate the same assembly so we -// choose the one that adheres to the standard. -// -// -// WHY HAVE typedef void MoveOnlyTypeForCPP03 -// -// Callback<>/Bind() needs to understand movable-but-not-copyable semantics -// to call .Pass() appropriately when it is expected to transfer the value. -// The cryptic typedef MoveOnlyTypeForCPP03 is added to make this check -// easy and automatic in helper templates for Callback<>/Bind(). -// See IsMoveOnlyType template and its usage in butil/callback_internal.h -// for more details. -// -// -// COMPARED TO C++11 -// -// In C++11, you would implement this functionality using an r-value reference -// and our .Pass() method would be replaced with a call to std::move(). -// -// This emulation also has a deficiency where it uses up the single -// user-defined conversion allowed by C++ during initialization. This can -// cause problems in some API edge cases. For instance, in scoped_ptr, it is -// impossible to make a function "void Foo(scoped_ptr p)" accept a -// value of type scoped_ptr even if you add a constructor to -// scoped_ptr<> that would make it look like it should work. C++11 does not -// have this deficiency. -// -// -// COMPARED TO Boost.Move -// -// Our implementation similar to Boost.Move, but we keep the RValue struct -// private to the move-only type, and we don't use the reinterpret_cast<> hack. -// -// In Boost.Move, RValue is the boost::rv<> template. This type can be used -// when writing APIs like: -// -// void MyFunc(boost::rv& f) -// -// that can take advantage of rv<> to avoid extra copies of a type. However you -// would still be able to call this version of MyFunc with an l-value: -// -// Foo f; -// MyFunc(f); // Uh oh, we probably just destroyed |f| w/o calling Pass(). -// -// unless someone is very careful to also declare a parallel override like: -// -// void MyFunc(const Foo& f) -// -// that would catch the l-values first. This was declared unsafe in C++11 and -// a C++11 compiler will explicitly fail MyFunc(f). Unfortunately, we cannot -// ensure this in C++03. -// -// Since we have no need for writing such APIs yet, our implementation keeps -// RValue private and uses a .Pass() method to do the conversion instead of -// trying to write a version of "std::move()." Writing an API like std::move() -// would require the RValue struct to be public. -// -// -// CAVEATS -// -// If you include a move-only type as a field inside a class that does not -// explicitly declare a copy constructor, the containing class's implicit -// copy constructor will change from Containing(const Containing&) to -// Containing(Containing&). This can cause some unexpected errors. -// -// http://llvm.org/bugs/show_bug.cgi?id=11528 -// -// The workaround is to explicitly declare your copy constructor. -// -#define MOVE_ONLY_TYPE_FOR_CPP_03(type, rvalue_type) \ - private: \ - struct rvalue_type { \ - explicit rvalue_type(type* object) : object(object) {} \ - type* object; \ - }; \ - type(type&); \ - void operator=(type&); \ - public: \ - operator rvalue_type() { return rvalue_type(this); } \ - type Pass() { return type(rvalue_type(this)); } \ - typedef void MoveOnlyTypeForCPP03; \ - private: - -#endif // BUTIL_MOVE_H_ diff --git a/src/butil/scoped_generic.h b/src/butil/scoped_generic.h index d92c095b4a..96de4123a7 100644 --- a/src/butil/scoped_generic.h +++ b/src/butil/scoped_generic.h @@ -8,9 +8,9 @@ #include #include +#include #include "butil/compiler_specific.h" -#include "butil/move.h" #include "butil/macros.h" namespace butil { @@ -54,8 +54,6 @@ namespace butil { // typedef ScopedGeneric ScopedFoo; template class ScopedGeneric { - MOVE_ONLY_TYPE_FOR_CPP_03(ScopedGeneric, RValue) - private: // This must be first since it's used inline below. // @@ -84,11 +82,15 @@ class ScopedGeneric { : data_(value, traits) { } - // Move constructor for C++03 move emulation. - ScopedGeneric(RValue rvalue) - : data_(rvalue.object->release(), rvalue.object->get_traits()) { + ScopedGeneric(const ScopedGeneric&) = delete; + ScopedGeneric& operator=(const ScopedGeneric&) = delete; + + ScopedGeneric(ScopedGeneric&& other) + : data_(other.release(), other.get_traits()) { } + ScopedGeneric& operator=(ScopedGeneric&&) = delete; + ~ScopedGeneric() { FreeIfNecessary(); } diff --git a/test/callback_list_unittest.cc b/test/callback_list_unittest.cc index 422bb08cd8..d4987b1f3c 100644 --- a/test/callback_list_unittest.cc +++ b/test/callback_list_unittest.cc @@ -4,6 +4,8 @@ #include "butil/callback_list.h" +#include + #include "butil/basictypes.h" #include "butil/bind.h" #include "butil/bind_helpers.h" @@ -37,7 +39,7 @@ class Remover { } void SetSubscriptionToRemove( scoped_ptr::Subscription> sub) { - removal_subscription_ = sub.Pass(); + removal_subscription_ = std::move(sub); } int total() const { return total_; } @@ -234,9 +236,9 @@ TEST(CallbackListTest, RemoveCallbacksDuringIteration) { cb_reg.Add(Bind(&Listener::IncrementTotal, Unretained(&b))); // |remover_1| will remove itself. - remover_1.SetSubscriptionToRemove(remover_1_sub.Pass()); + remover_1.SetSubscriptionToRemove(std::move(remover_1_sub)); // |remover_2| will remove a. - remover_2.SetSubscriptionToRemove(a_subscription.Pass()); + remover_2.SetSubscriptionToRemove(std::move(a_subscription)); cb_reg.Notify(); diff --git a/test/environment_unittest.cc b/test/environment_unittest.cc index a5b6fcb969..6b79f711cc 100644 --- a/test/environment_unittest.cc +++ b/test/environment_unittest.cc @@ -129,7 +129,7 @@ TEST_F(EnvironmentTest, AlterEnvironment) { EnvironmentMap changes; scoped_ptr e; - e = AlterEnvironment(empty, changes).Pass(); + e = AlterEnvironment(empty, changes); EXPECT_TRUE(e[0] == nullptr); changes["A"] = "1"; diff --git a/test/file_unittest.cc b/test/file_unittest.cc index e25d903221..682c06e06e 100644 --- a/test/file_unittest.cc +++ b/test/file_unittest.cc @@ -6,11 +6,90 @@ #include "butil/files/file.h" #include "butil/files/scoped_temp_dir.h" #include "butil/time/time.h" +#if defined(OS_POSIX) +#include +#include +#endif +#include +#include +#include #include using butil::File; using butil::FilePath; +static_assert(!std::is_copy_constructible::value, + "File must not be copy constructible"); +static_assert(!std::is_copy_assignable::value, + "File must not be copy assignable"); +static_assert(std::is_move_constructible::value, + "File must be move constructible"); +static_assert(std::is_move_assignable::value, + "File must be move assignable"); +static_assert(std::is_nothrow_move_constructible::value, + "File move construction must be noexcept"); +static_assert(std::is_nothrow_move_assignable::value, + "File move assignment must be noexcept"); +static_assert(!std::is_copy_constructible::value, + "ScopedFILE must not be copy constructible"); +static_assert(!std::is_copy_assignable::value, + "ScopedFILE must not be copy assignable"); +static_assert(std::is_move_constructible::value, + "ScopedFILE must be move constructible"); +static_assert(!std::is_move_assignable::value, + "ScopedFILE must remain construct-only"); + +TEST(FileTest, MoveTransfersHandleAndMetadata) { + butil::ScopedTempDir temp_dir; + ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); + FilePath path = temp_dir.path().AppendASCII("move_file"); + File source(path, File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE | + File::FLAG_ASYNC); + ASSERT_TRUE(source.IsValid()); + const butil::PlatformFile handle = source.GetPlatformFile(); + + File constructed(std::move(source)); + EXPECT_FALSE(source.IsValid()); + EXPECT_EQ(handle, constructed.GetPlatformFile()); + EXPECT_EQ(File::FILE_OK, constructed.error_details()); + EXPECT_TRUE(constructed.created()); + EXPECT_TRUE(constructed.async()); + + FilePath target_path = temp_dir.path().AppendASCII("move_target_file"); + File assigned(target_path, + File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE); + ASSERT_TRUE(assigned.IsValid()); + const butil::PlatformFile old_target_handle = assigned.GetPlatformFile(); + assigned = std::move(constructed); + EXPECT_FALSE(constructed.IsValid()); + EXPECT_EQ(handle, assigned.GetPlatformFile()); + EXPECT_EQ(File::FILE_OK, assigned.error_details()); + EXPECT_TRUE(assigned.created()); + EXPECT_TRUE(assigned.async()); +#if defined(OS_POSIX) + errno = 0; + EXPECT_EQ(-1, fcntl(old_target_handle, F_GETFD)); + EXPECT_EQ(EBADF, errno); +#endif + + auto& same_owner = assigned; + assigned = std::move(same_owner); + EXPECT_TRUE(assigned.IsValid()); + EXPECT_EQ(handle, assigned.GetPlatformFile()); + EXPECT_EQ(File::FILE_OK, assigned.error_details()); + EXPECT_TRUE(assigned.created()); + EXPECT_TRUE(assigned.async()); +} + +TEST(FileTest, ScopedFILEMoveConstructionTransfersOwnership) { + FILE* raw = std::tmpfile(); + ASSERT_NE(nullptr, raw); + butil::ScopedFILE source(raw); + butil::ScopedFILE moved(std::move(source)); + EXPECT_EQ(nullptr, source.get()); + EXPECT_EQ(raw, moved.get()); +} + TEST(FileTest, Create) { butil::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); @@ -240,7 +319,7 @@ TEST(FileTest, Append) { ASSERT_TRUE(file2.IsValid()); // Test passing the file around. - file = file2.Pass(); + file = std::move(file2); EXPECT_FALSE(file2.IsValid()); ASSERT_TRUE(file.IsValid()); diff --git a/test/file_util_unittest.cc b/test/file_util_unittest.cc index 736ba0566d..029eee64c1 100644 --- a/test/file_util_unittest.cc +++ b/test/file_util_unittest.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "butil/file_util.h" @@ -1713,14 +1714,14 @@ TEST_F(FileUtilTest, CreateAndOpenTemporaryFileTest) { TEST_F(FileUtilTest, FileToFILE) { File file; - FILE* stream = FileToFILE(file.Pass(), "w"); + FILE* stream = FileToFILE(std::move(file), "w"); EXPECT_FALSE(stream); FilePath file_name = temp_dir_.path().Append(FPL("The file.txt")); file = File(file_name, File::FLAG_CREATE | File::FLAG_WRITE); EXPECT_TRUE(file.IsValid()); - stream = FileToFILE(file.Pass(), "w"); + stream = FileToFILE(std::move(file), "w"); EXPECT_TRUE(stream); EXPECT_FALSE(file.IsValid()); EXPECT_TRUE(CloseFile(stream)); diff --git a/test/scoped_generic_unittest.cc b/test/scoped_generic_unittest.cc index 4f707c605b..4ad112bf13 100644 --- a/test/scoped_generic_unittest.cc +++ b/test/scoped_generic_unittest.cc @@ -3,6 +3,8 @@ // found in the LICENSE file. #include +#include +#include #include "butil/scoped_generic.h" #include @@ -26,6 +28,15 @@ struct IntTraits { typedef ScopedGeneric ScopedInt; +static_assert(!std::is_copy_constructible::value, + "ScopedGeneric must not be copy constructible"); +static_assert(!std::is_copy_assignable::value, + "ScopedGeneric must not be copy assignable"); +static_assert(std::is_move_constructible::value, + "ScopedGeneric must be move constructible"); +static_assert(!std::is_move_assignable::value, + "ScopedGeneric must remain construct-only"); + } // namespace TEST(ScopedGenericTest, ScopedGeneric) { @@ -85,13 +96,14 @@ TEST(ScopedGenericTest, ScopedGeneric) { EXPECT_EQ(kSecond, values_freed[1]); values_freed.clear(); - // Pass. + // Move construction preserves ownership and copies stateful traits. { ScopedInt a(kFirst, traits); - ScopedInt b(a.Pass()); + ScopedInt b(std::move(a)); EXPECT_TRUE(values_freed.empty()); // Nothing should be freed. ASSERT_EQ(IntTraits::InvalidValue(), a.get()); ASSERT_EQ(kFirst, b.get()); + EXPECT_EQ(&values_freed, b.get_traits().freed_ints); } ASSERT_EQ(1u, values_freed.size()); ASSERT_EQ(kFirst, values_freed[0]); diff --git a/test/scoped_ptr_unittest.cc b/test/scoped_ptr_unittest.cc index 86af29e38f..083693da5d 100644 --- a/test/scoped_ptr_unittest.cc +++ b/test/scoped_ptr_unittest.cc @@ -4,6 +4,9 @@ #include "butil/memory/scoped_ptr.h" +#include +#include + #include "butil/basictypes.h" #include @@ -35,6 +38,9 @@ class ConDecLogger : public ConDecLoggerParent { DISALLOW_COPY_AND_ASSIGN(ConDecLogger); }; +struct ArrayBase {}; +struct ArrayDerived : ArrayBase {}; + struct CountingDeleter { explicit CountingDeleter(int* count) : count_(count) {} inline void operator()(double* ptr) const { @@ -77,7 +83,7 @@ int OverloadedNewAndDelete::g_new_count = 0; int OverloadedNewAndDelete::g_delete_count = 0; scoped_ptr PassThru(scoped_ptr logger) { - return logger.Pass(); + return logger; } void GrabAndDrop(scoped_ptr logger) { @@ -89,11 +95,33 @@ scoped_ptr TestReturnOfType(int* constructed) { return scoped_ptr(new ConDecLogger(constructed)); } -scoped_ptr UpcastUsingPassAs( +scoped_ptr UpcastOnReturn( scoped_ptr object) { - return object.PassAs(); + return object; } +static_assert(std::is_move_constructible>::value, + "scoped_ptr must be move constructible"); +static_assert(std::is_move_assignable>::value, + "scoped_ptr must be move assignable"); +static_assert(!std::is_copy_constructible>::value, + "scoped_ptr must not be copy constructible"); +static_assert(!std::is_copy_assignable>::value, + "scoped_ptr must not be copy assignable"); +static_assert( + std::is_constructible, + scoped_ptr&&>::value, + "scoped_ptr must support safe pointer conversions"); +static_assert( + std::is_constructible, + scoped_ptr&&>::value, + "scoped_ptr must support adding const"); +static_assert(std::is_move_constructible>::value, + "array scoped_ptr must be move constructible"); +static_assert(!std::is_constructible, + scoped_ptr&&>::value, + "array scoped_ptr must reject derived-to-base conversion"); + } // namespace TEST(ScopedPtrTest, ScopedPtr) { @@ -176,7 +204,7 @@ TEST(ScopedPtrTest, ScopedPtrDepthSubtyping) { EXPECT_EQ(1, constructed); EXPECT_TRUE(scoper.get()); - scoped_ptr scoper_parent(scoper.Pass()); + scoped_ptr scoper_parent(std::move(scoper)); EXPECT_EQ(1, constructed); EXPECT_TRUE(scoper_parent.get()); EXPECT_FALSE(scoper.get()); @@ -194,7 +222,7 @@ TEST(ScopedPtrTest, ScopedPtrDepthSubtyping) { EXPECT_TRUE(scoper.get()); scoped_ptr scoper_parent; - scoper_parent = scoper.Pass(); + scoper_parent = std::move(scoper); EXPECT_EQ(1, constructed); EXPECT_TRUE(scoper_parent.get()); EXPECT_FALSE(scoper.get()); @@ -207,7 +235,7 @@ TEST(ScopedPtrTest, ScopedPtrDepthSubtyping) { EXPECT_EQ(1, constructed); EXPECT_TRUE(scoper.get()); - scoped_ptr scoper_const(scoper.Pass()); + scoped_ptr scoper_const(std::move(scoper)); EXPECT_EQ(1, constructed); EXPECT_TRUE(scoper_const.get()); EXPECT_FALSE(scoper.get()); @@ -225,7 +253,7 @@ TEST(ScopedPtrTest, ScopedPtrDepthSubtyping) { EXPECT_TRUE(scoper.get()); scoped_ptr scoper_const; - scoper_const = scoper.Pass(); + scoper_const = std::move(scoper); EXPECT_EQ(1, constructed); EXPECT_TRUE(scoper_const.get()); EXPECT_FALSE(scoper.get()); @@ -249,7 +277,7 @@ TEST(ScopedPtrTest, ScopedPtrDepthSubtyping) { EXPECT_EQ(0, alternate_deletes); // Test this compiles and correctly overwrites the deleter state. - scoper = scoper_child.Pass(); + scoper = std::move(scoper_child); EXPECT_TRUE(scoper); EXPECT_FALSE(scoper_child); EXPECT_EQ(1, deletes); @@ -265,7 +293,8 @@ TEST(ScopedPtrTest, ScopedPtrDepthSubtyping) { EXPECT_TRUE(scoper_child); EXPECT_EQ(1, deletes); EXPECT_EQ(1, alternate_deletes); - scoped_ptr scoper_construct(scoper_child.Pass()); + scoped_ptr scoper_construct( + std::move(scoper_child)); EXPECT_TRUE(scoper_construct); EXPECT_FALSE(scoper_child); EXPECT_EQ(1, deletes); @@ -361,13 +390,13 @@ TEST(ScopedPtrTest, ScopedPtrWithArray) { } EXPECT_EQ(kNumLoggers, constructed); - // Test Pass() with constructor; - scoped_ptr scoper2(scoper.Pass()); + // Test move construction. + scoped_ptr scoper2(std::move(scoper)); EXPECT_EQ(kNumLoggers, constructed); - // Test Pass() with assignment; + // Test move assignment. scoped_ptr scoper3; - scoper3 = scoper2.Pass(); + scoper3 = std::move(scoper2); EXPECT_EQ(kNumLoggers, constructed); EXPECT_FALSE(scoper); EXPECT_FALSE(scoper2); @@ -376,34 +405,34 @@ TEST(ScopedPtrTest, ScopedPtrWithArray) { EXPECT_EQ(0, constructed); } -TEST(ScopedPtrTest, PassBehavior) { +TEST(ScopedPtrTest, MoveBehavior) { int constructed = 0; { ConDecLogger* logger = new ConDecLogger(&constructed); scoped_ptr scoper(logger); EXPECT_EQ(1, constructed); - // Test Pass() with constructor; - scoped_ptr scoper2(scoper.Pass()); + // Test move construction. + scoped_ptr scoper2(std::move(scoper)); EXPECT_EQ(1, constructed); - // Test Pass() with assignment; + // Test move assignment. scoped_ptr scoper3; - scoper3 = scoper2.Pass(); + scoper3 = std::move(scoper2); EXPECT_EQ(1, constructed); EXPECT_FALSE(scoper.get()); EXPECT_FALSE(scoper2.get()); EXPECT_TRUE(scoper3.get()); } - // Test uncaught Pass() does not leak. + // Explicit reset releases ownership immediately. { ConDecLogger* logger = new ConDecLogger(&constructed); scoped_ptr scoper(logger); EXPECT_EQ(1, constructed); - // Should auto-destruct logger by end of scope. - scoper.Pass(); + scoper.reset(); + EXPECT_EQ(0, constructed); EXPECT_FALSE(scoper.get()); } EXPECT_EQ(0, constructed); @@ -415,12 +444,34 @@ TEST(ScopedPtrTest, PassBehavior) { EXPECT_EQ(1, constructed); // Should auto-destruct logger by end of scope. - GrabAndDrop(scoper.Pass()); + GrabAndDrop(std::move(scoper)); EXPECT_FALSE(scoper.get()); } EXPECT_EQ(0, constructed); } +TEST(ScopedPtrTest, SelfMoveAssignmentPreservesOwnership) { + int constructed = 0; + scoped_ptr scoper(new ConDecLogger(&constructed)); + ConDecLogger* original = scoper.get(); + + auto& same_owner = scoper; + scoper = std::move(same_owner); + + EXPECT_EQ(original, scoper.get()); + EXPECT_EQ(1, constructed); +} + +TEST(ScopedPtrTest, ArraySelfMoveAssignmentPreservesOwnership) { + scoped_ptr scoper(new int[2]); + int* original = scoper.get(); + + auto& same_owner = scoper; + scoper = std::move(same_owner); + + EXPECT_EQ(original, scoper.get()); +} + TEST(ScopedPtrTest, ReturnTypeBehavior) { int constructed = 0; @@ -430,7 +481,7 @@ TEST(ScopedPtrTest, ReturnTypeBehavior) { scoped_ptr scoper(logger); EXPECT_EQ(1, constructed); - PassThru(scoper.Pass()); + PassThru(std::move(scoper)); EXPECT_FALSE(scoper.get()); } EXPECT_EQ(0, constructed); @@ -442,7 +493,7 @@ TEST(ScopedPtrTest, ReturnTypeBehavior) { EXPECT_EQ(1, constructed); // Should auto-destruct logger by end of scope. - PassThru(scoper.Pass()); + PassThru(std::move(scoper)); EXPECT_FALSE(scoper.get()); } EXPECT_EQ(0, constructed); @@ -455,7 +506,7 @@ TEST(ScopedPtrTest, ReturnTypeBehavior) { EXPECT_EQ(0, constructed); } -TEST(ScopedPtrTest, PassAs) { +TEST(ScopedPtrTest, ConvertingMoveOnReturn) { int constructed = 0; { scoped_ptr scoper(new ConDecLogger(&constructed)); @@ -463,7 +514,7 @@ TEST(ScopedPtrTest, PassAs) { EXPECT_TRUE(scoper.get()); scoped_ptr scoper_parent; - scoper_parent = UpcastUsingPassAs(scoper.Pass()); + scoper_parent = UpcastOnReturn(std::move(scoper)); EXPECT_EQ(1, constructed); EXPECT_TRUE(scoper_parent.get()); EXPECT_FALSE(scoper.get()); @@ -524,7 +575,7 @@ TEST(ScopedPtrTest, CustomDeleter) { scoper.get_deleter()(&dummy_value); EXPECT_EQ(3, deletes); - // Deleter can be assigned into (matches C++11 unique_ptr<> spec). + // Deleter can be assigned into (matches the std::unique_ptr spec). scoper.get_deleter() = CountingDeleter(&alternate_deletes); scoper.reset(&dummy_value); EXPECT_EQ(0, alternate_deletes); @@ -546,11 +597,11 @@ TEST(ScopedPtrTest, CustomDeleter) { EXPECT_EQ(0, deletes); EXPECT_EQ(0, alternate_deletes); - // Pass the second deleter through a constructor and an operator=. Then + // Move the second deleter through a constructor and an operator=. Then // reinitialize the empty scopers to ensure that each one is deleting // properly. - scoped_ptr scoper3(scoper2.Pass()); - scoper = scoper3.Pass(); + scoped_ptr scoper3(std::move(scoper2)); + scoper = std::move(scoper3); EXPECT_EQ(1, deletes); scoper2.reset(&dummy_value2); @@ -587,7 +638,7 @@ TEST(ScopedPtrTest, CustomDeleter) { } // Sanity check test for overloaded new and delete operators. Does not do full -// coverage of reset/release/Pass() operations as that is redundant with the +// coverage of reset/release/move operations as that is redundant with the // above. TEST(ScopedPtrTest, OverloadedNewAndDelete) { { @@ -595,7 +646,7 @@ TEST(ScopedPtrTest, OverloadedNewAndDelete) { scoped_ptr scoper(new OverloadedNewAndDelete()); EXPECT_TRUE(scoper.get()); - scoped_ptr scoper2(scoper.Pass()); + scoped_ptr scoper2(std::move(scoper)); } EXPECT_EQ(1, OverloadedNewAndDelete::delete_count()); EXPECT_EQ(1, OverloadedNewAndDelete::new_count()); diff --git a/test/scoped_vector_unittest.cc b/test/scoped_vector_unittest.cc index 415edec03e..0c93b4bda4 100644 --- a/test/scoped_vector_unittest.cc +++ b/test/scoped_vector_unittest.cc @@ -4,11 +4,24 @@ #include "butil/memory/scoped_vector.h" +#include +#include + #include "butil/memory/scoped_ptr.h" #include namespace { +static_assert(!std::is_copy_constructible >::value, + "ScopedVector must not be copy constructible"); +static_assert(!std::is_copy_assignable >::value, + "ScopedVector must not be copy assignable"); +static_assert(std::is_nothrow_move_constructible< + butil::ScopedVector >::value, + "ScopedVector move construction must be noexcept"); +static_assert(std::is_move_assignable >::value, + "ScopedVector must be move assignable"); + // The LifeCycleObject notifies its Observer upon construction & destruction. class LifeCycleObject { public: @@ -223,7 +236,8 @@ TEST(ScopedVectorTest, MoveConstruct) { EXPECT_FALSE(scoped_vector.empty()); EXPECT_TRUE(watcher.IsWatching(scoped_vector.back())); - butil::ScopedVector scoped_vector_copy(scoped_vector.Pass()); + butil::ScopedVector scoped_vector_copy( + std::move(scoped_vector)); EXPECT_TRUE(scoped_vector.empty()); EXPECT_FALSE(scoped_vector_copy.empty()); EXPECT_TRUE(watcher.IsWatching(scoped_vector_copy.back())); @@ -243,7 +257,7 @@ TEST(ScopedVectorTest, MoveAssign) { EXPECT_FALSE(scoped_vector.empty()); EXPECT_TRUE(watcher.IsWatching(scoped_vector.back())); - scoped_vector_assign = scoped_vector.Pass(); + scoped_vector_assign = std::move(scoped_vector); EXPECT_TRUE(scoped_vector.empty()); EXPECT_FALSE(scoped_vector_assign.empty()); EXPECT_TRUE(watcher.IsWatching(scoped_vector_assign.back())); @@ -271,24 +285,75 @@ class DeleteCounter { DISALLOW_COPY_AND_ASSIGN(DeleteCounter); }; +TEST(ScopedVectorTest, MoveAssignDeletesPopulatedTarget) { + int source_deletes = 0; + int target_deletes = 0; + { + butil::ScopedVector source; + source.push_back(new DeleteCounter(&source_deletes)); + butil::ScopedVector target; + target.push_back(new DeleteCounter(&target_deletes)); + + target = std::move(source); + + EXPECT_TRUE(source.empty()); + ASSERT_EQ(1u, target.size()); + EXPECT_EQ(0, source_deletes); + EXPECT_EQ(1, target_deletes); + } + EXPECT_EQ(1, source_deletes); + EXPECT_EQ(1, target_deletes); +} + +TEST(ScopedVectorTest, SelfMoveAssignmentPreservesOwnership) { + int deletes = 0; + { + butil::ScopedVector vector; + vector.push_back(new DeleteCounter(&deletes)); + + auto& same_owner = vector; + vector = std::move(same_owner); + + ASSERT_EQ(1u, vector.size()); + EXPECT_EQ(0, deletes); + } + EXPECT_EQ(1, deletes); +} + template butil::ScopedVector PassThru(butil::ScopedVector scoper) { - return scoper.Pass(); + return scoper; } -TEST(ScopedVectorTest, Passed) { +TEST(ScopedVectorTest, MovedOwnerCanBeCleared) { int deletes = 0; butil::ScopedVector deleter_vector; deleter_vector.push_back(new DeleteCounter(&deletes)); EXPECT_EQ(0, deletes); EXPECT_EQ(0, deletes); - butil::ScopedVector result = deleter_vector.Pass(); + butil::ScopedVector result = std::move(deleter_vector); EXPECT_EQ(0, deletes); result.clear(); EXPECT_EQ(1, deletes); }; +TEST(ScopedVectorTest, ImplicitMoveOnReturn) { + int deletes = 0; + { + butil::ScopedVector source; + source.push_back(new DeleteCounter(&deletes)); + + butil::ScopedVector result = + PassThru(std::move(source)); + + EXPECT_TRUE(source.empty()); + ASSERT_EQ(1u, result.size()); + EXPECT_EQ(0, deletes); + } + EXPECT_EQ(1, deletes); +} + TEST(ScopedVectorTest, InsertRange) { LifeCycleWatcher watchers[5];