Copilot commented on code in PR #3685:
URL: https://github.com/apache/brpc/pull/3685#discussion_r4443407091
##########
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<Foo> arg) {
// // Do something with arg
// }
// scoped_ptr<Foo> CreateFoo() {
-// // No need for calling Pass() because we are constructing a temporary
-// // for the return value.
// return scoped_ptr<Foo>(new Foo("new"));
// }
// scoped_ptr<Foo> PassThru(scoped_ptr<Foo> arg) {
-// return arg.Pass();
+// return arg;
// }
//
// {
// scoped_ptr<Foo> 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<Foo> ptr2 = CreateFoo(); // ptr2 owns the return Foo.
// scoped_ptr<Foo> 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<Child> to initialize a scoped_ptr<Parent>:
//
// scoped_ptr<Foo> foo(new Foo());
-// scoped_ptr<FooParent> parent(foo.Pass());
+// scoped_ptr<FooParent> 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:
Review Comment:
This comment attributes the upcast behavior to `std::move()`, but
`std::move` only casts to an rvalue; the actual upcast is enabled by
`scoped_ptr`'s converting move constructor/assignment. Rewording to reflect
that `std::move` triggers `scoped_ptr`'s converting move (and that
return-by-value can implicitly move) will make the guidance more accurate for
downstream users.
##########
src/butil/scoped_generic.h:
##########
@@ -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()) {
Review Comment:
The move constructor initializes `data_` using two calls on `other`
(`release()` and `get_traits()`) in the same initializer expression. The
evaluation order of constructor initializer arguments is not guaranteed prior
to C++17, which can lead to fragile behavior if `release()` ever changes state
needed by `get_traits()` (or vice versa). To make this sequencing explicit and
robust, capture the traits (or a copy of them) and the released value in
temporaries before initializing `data_`.
##########
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);
+ }
return *this;
}
Review Comment:
Move assignment is not marked `noexcept`, even though the implementation
(clearing pointer elements + `std::vector` swap for the default allocator)
should be non-throwing. Marking move assignment `noexcept` (or conditionally
`noexcept(...)`) can improve performance in generic code and reduce
pessimization when `ScopedVector` is used in higher-level move-heavy paths.
--
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]
