Repository navigation
refactor(butil): Adopt native move semantics - #3685
darion-yaphet wants to merge 4 commits into
Conversation
Replace C++03 move emulation with native rvalue references and std::move for butil resource owners. Update callers, add ownership-transfer regression coverage, and document downstream migration. BREAKING CHANGE: Remove butil/move.h, MOVE_ONLY_TYPE_FOR_CPP_03, Pass(), and PassAs(). File move signatures now take File&&; downstream shared-library consumers must rebuild.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
What changed in this PR
Refactors butil’s ownership-transfer types from C++03 move emulation (Pass(), MOVE_ONLY_TYPE_FOR_CPP_03) to native C++11/14 move semantics using rvalue references and std::move, and updates tests/docs accordingly.
Changes:
- Replace legacy move emulation with deleted copy operations + native move constructors/assignments for
scoped_ptr,ScopedVector,ScopedGeneric,File, andScopedFILE. - Update ownership-transfer call sites (
Pass()/PassAs()) tostd::move(...)and return-by-value patterns. - Add/extend regression coverage and document downstream migration steps.
| File | Description |
|---|---|
| test/scoped_vector_unittest.cc | Updates Pass() usage to std::move and adds regression tests/static-asserts for move semantics. |
| test/scoped_ptr_unittest.cc | Migrates tests off Pass()/PassAs(), adds compile-time assertions and self-move regression tests. |
| test/scoped_generic_unittest.cc | Switches to move construction and validates traits/state behavior. |
| test/file_util_unittest.cc | Updates FileToFILE callers to pass File via std::move. |
| test/file_unittest.cc | Adds move-semantics tests/static-asserts for File and ScopedFILE; updates ownership transfer from Pass() to std::move. |
| test/environment_unittest.cc | Removes Pass() call site now that return-by-value moves. |
| test/callback_list_unittest.cc | Replaces Pass() with std::move for subscription ownership transfer. |
| src/butil/scoped_generic.h | Removes C++03 move emulation macro; adds deleted copy ops + move ctor. |
| src/butil/move.h | Deletes the legacy C++03 move emulation header. |
| src/butil/memory/scoped_vector.h | Removes move emulation macro; adds native move ctor/assignment with new semantics. |
| src/butil/memory/scoped_ptr.h | Removes move emulation + PassAs; adds native move operations, SFINAE for safe converting moves, updates docs/comments. |
| src/butil/files/scoped_file.h | Migrates ScopedFILE to native move construction (construct-only). |
| src/butil/files/memory_mapped_file.cc | Replaces Pass() with std::move for file ownership transfer. |
| src/butil/files/file.h | Replaces C++03 emulated move signatures with File&& move ctor/assignment. |
| src/butil/files/file.cc | Implements File(File&&) and operator=(File&&) using native moves. |
| src/butil/environment.cc | Returns scoped_ptr by value (implicit move) instead of Pass(). |
| src/butil/containers/scoped_ptr_hash_map.h | Replaces Pass() with return-by-value moves for take-style APIs. |
| src/bthread/execution_queue_inl.h | Replaces Pass() with return-by-value for scoped pointer returns. |
| src/bthread/execution_queue.cpp | Replaces Pass() with return-by-value for scoped pointer returns. |
| docs/en/getting_started.md | Adds pointer to the C++14 move migration guide. |
| docs/en/cpp14_move.md | Adds downstream migration documentation for the move-semantics refactor. |
| BUILD.bazel | Removes src/butil/move.h from build inputs. |
Review effort: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The change affects public headers, exported File symbols, and platform-specific handle behavior, while validation covers only macOS, leaving Windows builds and downstream ABI/source compatibility requiring human verification.
3 open issues
Review effort: Lite
Clarify that std::move only casts a named owner to an rvalue and that scoped_ptr performs upcasting through its converting move constructor.
There was a problem hiding this comment.
🔵 Needs a closer look
The public ownership/API and exported File ABI changes, including header removal, are supported only by macOS/POSIX validation, leaving downstream migration and other platforms for human confirmation.
No open issues 🎉
3 resolved since last review
Review effort: Lite
|
The remaining risk is intentional and already called out in the PR:
|
| COMPILE_ASSERT(!butil::is_array<U>::value, U_cannot_be_an_array); | ||
| scoped_ptr& operator=(scoped_ptr&& rhs) { | ||
| if (this != &rhs) { | ||
| impl_.TakeState(&rhs.impl_); |
There was a problem hiding this comment.
When the move source is a member of an object owned by the destination, for example head = std::move(head->next) with a stateful deleter, rhs is destroyed during assignment: TakeState first deletes the old node via reset(other->release()), then reads other->get_deleter() from that node, so copying the deleter accesses freed memory. The original Pass() and by-value parameter first established an independent temporary owner, avoiding this issue. Please preserve the source's complete ownership state before clearing the old destination, and add a regression test for a nested source; converting assignment uses the same path.
Save the source ownership state before destroying the destination resources. Add regression tests for nested scoped_ptr and ScopedVector moves.
| File(const File&) = delete; | ||
| File& operator=(const File&) = delete; |
There was a problem hiding this comment.
Please use DISALLOW_COPY_AND_ASSIGN instead.
There was a problem hiding this comment.
Updated all five declarations to use DISALLOW_COPY_AND_ASSIGN.
| scoped_ptr(const scoped_ptr&) = delete; | ||
| scoped_ptr& operator=(const scoped_ptr&) = delete; |
There was a problem hiding this comment.
Please use DISALLOW_COPY_AND_ASSIGN instead.
| scoped_ptr(const scoped_ptr&) = delete; | ||
| scoped_ptr& operator=(const scoped_ptr&) = delete; |
There was a problem hiding this comment.
Please use DISALLOW_COPY_AND_ASSIGN instead.
| ScopedVector(const ScopedVector&) = delete; | ||
| ScopedVector& operator=(const ScopedVector&) = delete; |
There was a problem hiding this comment.
Please use DISALLOW_COPY_AND_ASSIGN instead.
| ScopedGeneric(const ScopedGeneric&) = delete; | ||
| ScopedGeneric& operator=(const ScopedGeneric&) = delete; |
There was a problem hiding this comment.
Please use DISALLOW_COPY_AND_ASSIGN instead.
Summary
Replace C++03 move emulation with native rvalue references and
std::moveforscoped_ptr,ScopedVector,ScopedGeneric,File, andScopedFILE.Update ownership-transfer call sites, remove the legacy move header and helpers, and add regression coverage for ownership transfer, self-move, resource cleanup, and custom deleters. Document downstream migration in
docs/en/cpp14_move.md.Testing
Risk and Rollback
This introduces source and binary compatibility changes:
butil/move.h,MOVE_ONLY_TYPE_FOR_CPP_03,Pass(), andPassAs(). Downstream callers must migrate to native move operations.Filemove signatures to acceptFile&&. Downstream shared-library consumers must rebuild.ScopedVectormove assignment now destroys the destination's previous elements and leaves the source empty, instead of swapping their contents.Discarded
Pass()calls must become explicit cleanup operations; discardingstd::move(owner)does not release resources.Rollback requires reverting the corresponding downstream API migrations and rebuilding affected binaries.