darion-yaphet commented on code in PR #3685:
URL: https://github.com/apache/brpc/pull/3685#discussion_r4455210982
##########
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:
Thanks for catching this. I’ve updated both move assignments locally to
preserve the source in an independent temporary before releasing the
destination’s old resources.
I also added regression tests for the nested scoped_ptr with a stateful
deleter and the nested ScopedVector case; the changes are not pushed yet.
--
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]
