chenBright commented on code in PR #3685:
URL: https://github.com/apache/brpc/pull/3685#discussion_r4454244354
##########
src/butil/memory/scoped_ptr.h:
##########
@@ -326,37 +297,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 <typename U, typename V>
- scoped_ptr(scoped_ptr<U, V> other) : impl_(&other.impl_) {
- COMPILE_ASSERT(!butil::is_array<U>::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 <typename U, typename V>
- scoped_ptr& operator=(scoped_ptr<U, V> rhs) {
- 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_);
Review 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.
##########
src/butil/memory/scoped_vector.h:
##########
@@ -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<value_type> il) : v_(il) {}
-#endif
- ScopedVector& operator=(RValue rhs) {
- swap(*rhs.object);
+ ScopedVector& operator=(ScopedVector&& rhs) {
+ if (this != &rhs) {
+ clear();
+ swap(rhs);
Review Comment:
If the move source is inside an element owned by the destination, for
example `nodes = std::move(nodes[0]->children)`, `clear()` first deletes the
parent node, destroys `rhs`, and frees its child nodes. The subsequent
`swap(rhs)` therefore accesses a freed object. The old `children.Pass()` first
moved the child nodes into an independent temporary container, avoiding
premature destruction of the source. Please first move-construct a temporary
container, then swap it with the destination and clear the old elements, and
add a test for this parent-child container scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
