Re: [boost] Is self assignment test valid?
on Mon Sep 08 2008, "Niels Dekker - no return address" <noreply-AT-this.is.invalid> wrote:
Andrei Alexandrescu wrote:
JoshuaMaurice (at) gm...com wrote:
As an aside, the way I've generally done it is as follows, which I assume to be equivalent to the two previous examples.
struct T { T& operator= (T const& x) { T(x).swap(*this); return *this; } void swap(T& x); };
That's the politically correct version. It turns out that, when a function needs to make a copy of its argument, it better takes it by value in the first place. That way, if an rvalue is passed in, the compiler can directly fuse the rvalue to the parameter thus saving a copy.
Do you think this is still relevant when operator= is inline, and it's just doing "T(x).swap(*this)"?
Absolutely. The compiler is allowed to elide the implicit copy when the argument is an rvalue if the corresponding parameter is taken by-value. However, when you write T(x) and x is an lvalue (as it is inside the operator= above), the compiler is required to honor your explicit instruction to make a copy of x.
If so, I might consider submitting another ticket regarding boost::function, which still copy-assigns in the "politically correct" way at the moment. As so many of us do :-)
Could you do that for all the other libraries too? I'm serious, I've been meaning to make a sweep across the source code to fix this.
Thanks! Looking a the example of passing the result of MakeUrl() to the function Connect(const String& url), you wrote: "For a compiler to optimize away the copy, it has to do the Herculean job of (1) getting access to Connect's definition (hard with separately compiled modules), (2) parse Connect's definition to develop an understanding of it, and (3) alter Connect's behavior so that the temporary is fused with finalUrl."
Now is it still such a Herculean job for a compiler to do so for an /inline/ assignment operator that's only just swapping a temporary copy? Honestly, I haven't done any profiling on this, so I just hope you did so already :-)
Yes, it's still a fairly herculean job. The compiler would have to prove that the entire program works exactly the same way with or without the copy, proving that nothing about the address or identity of the new T is significant. While it's possible that someone might implement that optimization for some cases where T's copy constructor, swap, and destructor are all inlined, that level of understanding is not typically developed in optimizers. In the case of copy-elision for by-value arguments and return values, the compiler is explicitly allowed to _assume_ there is no semantic difference between the original rvalue and its copy. That's low-hanging fruit for a compiler writer, that pays huge dividends. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
In the case of copy-elision for by-value arguments and return values, the compiler is explicitly allowed to _assume_ there is no semantic difference between the original rvalue and its copy. That's low-hanging fruit for a compiler writer, that pays huge dividends.
So far I've found 7 copy assignment operators that could be improved by having their argument "by value", instead of "by const reference", at the following locations, in trunk/boost: - any.hpp(64): boost::any - function\function_template.hpp(916): boost::function - intrusive_ptr.hpp(114): intrusive_ptr - multi_index_container.hpp(269): multi_index_container - interprocess\allocators\adaptive_pool.hpp(136): adaptive_pool_base - spirit\home\classic\tree\common.hpp(101): spirit::tree_node - spirit\home\classic\tree\common.hpp(624): spirit::tree_match Am I correct, or am I possibly missing some others? I wouldn't mind creating some tickets (one per source file), requesting to have their argument "by value", as we discussed at comp.lang.c++.moderated, "Re: Is self assignment test valid?". Basically it was concluded that /if/ an assignment operator is implemented by means of copy-and-swap, it should preferably be done as follows: T& operator=(T arg) { arg.swap(*this); return *this; } And preferably /not/ as follows: T& operator=(const T & arg) { T(arg).swap(*this); return *this; } Kind regards, -- Niels Dekker http://www.xs4all.nl/~nd/dekkerware Scientific programmer at LKEB, Leiden University Medical Center
Niels Dekker:
Basically it was concluded that /if/ an assignment operator is implemented by means of copy-and-swap, it should preferably be done as follows:
T& operator=(T arg) { arg.swap(*this); return *this; }
This is well known (although the typical form is swap(arg) instead of arg.swap(*this)). It is not widely used in Boost because it creates problems with some compilers (I recall Borland being one). The "classic" form is, unsurprisingly, much better supported across the board. Things may have improved now, of course.
Peter Dimov wrote:
Niels Dekker:
Basically it was concluded that /if/ an assignment operator is implemented by means of copy-and-swap, it should preferably be done as follows:
T& operator=(T arg) { arg.swap(*this); return *this; }
This is well known (although the typical form is swap(arg) instead of arg.swap(*this)). It is not widely used in Boost because it creates problems with some compilers (I recall Borland being one). The "classic" form is, unsurprisingly, much better supported across the board. Things may have improved now, of course.
It also allows you to support move assignment emulation rather elegantly using the move library in the sandbox. Thanks, Michael Marcin
Michael Marcin: ...
T& operator=(T arg) { arg.swap(*this); return *this; }
...
It also allows you to support move assignment emulation rather elegantly using the move library in the sandbox.
There is no need to use a move library. The point of the above assignment is to take advantage of copy elision, which allows the compiler to construct the rvalue argument directly into 'arg', without a copy. In other words, if you have T t; T f(); int main() { t = f(); } the return value of f() can be constructed into 'arg', producing the equivalent of f().swap(t); If, in addition, the compiler implements RVO and f() is written in an RVO-friendly way, there will be no copies at all. (I'm sure that this is explained well in the clc++m thread, which I apologize for not having read.)
Peter Dimov wrote:
Michael Marcin: ...
It also allows you to support move assignment emulation rather elegantly using the move library in the sandbox.
There is no need to use a move library. The point of the above assignment is to take advantage of copy elision, which allows the compiler to construct the rvalue argument directly into 'arg', without a copy. In other words, if you have
For rvalues yes but for lvalues you need move emulation. Which can be implemented in the sandbox move library as follows. class foo { public: foo(); foo( T t ); foo( boost::move_from<foo> other ) { swap( other.source ); } foo& operator=( foo rhs ) { swap( rhs ); } void swap(); //... }; T bar(); int main() { foo x; foo y( bar() ); x = boost::move(y); } Thanks, Michael Marcin
On Wed, Sep 10, 2008 at 6:17 PM, Michael Marcin <mike.marcin@gmail.com> wrote:
Peter Dimov wrote:
Michael Marcin: ...
It also allows you to support move assignment emulation rather elegantly using the move library in the sandbox.
There is no need to use a move library. The point of the above assignment is to take advantage of copy elision, which allows the compiler to construct the rvalue argument directly into 'arg', without a copy. In other words, if you have
For rvalues yes but for lvalues you need move emulation. Which can be implemented in the sandbox move library as follows.
class foo { public: foo(); foo( T t ); foo( boost::move_from<foo> other ) { swap( other.source ); }
foo& operator=( foo rhs ) { swap( rhs ); }
void swap();
//... };
T bar();
int main() { foo x; foo y( bar() ); x = boost::move(y); }
If you do not want to deal with move emulation (which I've found very brittle in complex expressions), a simple way to to gain the advantage of T::operator=(T rhs) even when assigning from lvalues is something like: template<class T> T destructive_copy(T& x) { using std::swap; T result; swap(result, x); return x; } which relies on NRVO to eliminate expensive copies: struct foo { foo(); foo(const& foo); foo& operator(foo); friend swap(foo&, foo&); }; foo t1; // no expensive copies if the compiler // does both NRVO and temporaries // elision foo t2 (destructive_copy(t2)); foo t3; // again, no expensive copies t3 = destructive_copy(t2); The name is also something that is easily greppable for (I do not use 'move' to prevent name collisions with an eventual std::move or boost::move). For types which do not have an optimized swap is suboptimal, so some sfinae trick on is_swappable might be needed. Ah, of course it requires T to be DefaultConstructible, and most importantly CopyConstructible so it doesn't handle move only types. HTH, -- gpd
on Wed Sep 10 2008, "Giovanni Piero Deretta" <gpderetta-AT-gmail.com> wrote:
If you do not want to deal with move emulation (which I've found very brittle in complex expressions),
Do you mean the old-style move emulation that relied on T(T&) copy ctors (known to be brittle) or does this assessment apply also to the adobe-style emulation (http://stlab.adobe.com/group__move__related.html)? If so, could you explain in more detail?
a simple way to to gain the advantage of T::operator=(T rhs) even when assigning from lvalues is something like:
template<class T> T destructive_copy(T& x) { using std::swap; T result; swap(result, x); return x; }
This only works when T is both default constructible and has an optimized swap. The latter is detectable, roughly speaking, so the function could be altered to handle it, but the former is not, which it seems to me makes destructive_copy not very useful in generic code. ...
For types which do not have an optimized swap is suboptimal, so some sfinae trick on is_swappable might be needed. Ah, of course it requires T to be DefaultConstructible, and most importantly CopyConstructible so it doesn't handle move only types.
That's a lot of caveats. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
On Wed, Sep 10, 2008 at 9:30 PM, David Abrahams <dave@boostpro.com> wrote:
on Wed Sep 10 2008, "Giovanni Piero Deretta" <gpderetta-AT-gmail.com> wrote:
If you do not want to deal with move emulation (which I've found very brittle in complex expressions),
Do you mean the old-style move emulation that relied on T(T&) copy ctors (known to be brittle) or does this assessment apply also to the adobe-style emulation (http://stlab.adobe.com/group__move__related.html)? If so, could you explain in more detail?
The former; I have yet to try the latter. I got interested in the boost::move because it supported move only types (which IMHO are the really interesting application of move semantics). Too bad that doesn't seems to be a robust solution in C++0x.
a simple way to to gain the advantage of T::operator=(T rhs) even when assigning from lvalues is something like:
template<class T> T destructive_copy(T& x) { using std::swap; T result; swap(result, x); return x; }
This only works when T is both default constructible and has an optimized swap. The latter is detectable, roughly speaking, so the function could be altered to handle it, but the former is not, which it seems to me makes destructive_copy not very useful in generic code.
...
For types which do not have an optimized swap is suboptimal, so some sfinae trick on is_swappable might be needed. Ah, of course it requires T to be DefaultConstructible, and most importantly CopyConstructible so it doesn't handle move only types.
That's a lot of caveats.
I think that the 'adobe move' has the same requirements. OTOH, destructive_copy requires no specific support from the 'moved' type: a custom swap and the 'smart' operator= are useful on their own, so adding destructive_copy to one's set of tools requires very little extra baggage. -- gpd
on Wed Sep 10 2008, "Giovanni Piero Deretta" <gpderetta-AT-gmail.com> wrote:
On Wed, Sep 10, 2008 at 9:30 PM, David Abrahams <dave@boostpro.com> wrote:
on Wed Sep 10 2008, "Giovanni Piero Deretta" <gpderetta-AT-gmail.com> wrote:
If you do not want to deal with move emulation (which I've found very brittle in complex expressions),
Do you mean the old-style move emulation that relied on T(T&) copy ctors (known to be brittle) or does this assessment apply also to the adobe-style emulation (http://stlab.adobe.com/group__move__related.html)? If so, could you explain in more detail?
The former; I have yet to try the latter. I got interested in the boost::move because it supported move only types (which IMHO are the really interesting application of move semantics). Too bad that doesn't seems to be a robust solution in C++0x.
Move-only types aren't too interesting or difficult to write in C++0x, actually. Have you looked at http://svn.boost.org/trac/boost/browser/trunk/boost/ptr_container/detail/sta... ?
a simple way to to gain the advantage of T::operator=(T rhs) even when assigning from lvalues is something like:
template<class T> T destructive_copy(T& x) { using std::swap; T result; swap(result, x); return x; }
This only works when T is both default constructible and has an optimized swap. The latter is detectable, roughly speaking, so the function could be altered to handle it, but the former is not, which it seems to me makes destructive_copy not very useful in generic code.
...
For types which do not have an optimized swap is suboptimal, so some sfinae trick on is_swappable might be needed. Ah, of course it requires T to be DefaultConstructible, and most importantly CopyConstructible so it doesn't handle move only types.
That's a lot of caveats.
I think that the 'adobe move' has the same requirements.
Only because they wanted those requirements anyway.
OTOH, destructive_copy requires no specific support from the 'moved' type: a custom swap and the 'smart' operator= are useful on their own, so adding destructive_copy to one's set of tools requires very little extra baggage.
Very true. For what it's worth, I had been working in this direction to get move optimizations for C++03 when I discussed it with Daniel James a few months ago, and I presume you're aware of this thread: <http://news.gmane.org/find-root.php?message_id=%3cg8hj6n%24oud%241%40ger.gmane.org%3e>. However, I prefer my assign(x) = y idiom for handling assignment on non-move-or-elision-aware types. See attached. You can detect C++03 move/elision-awareness; combining that with this could be powerful. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
2008/9/10 David Abrahams <dave@boostpro.com>:
On Wed, Sep 10, 2008 at 9:30 PM, David Abrahams <dave@boostpro.com> wrote:
on Wed Sep 10 2008, "Giovanni Piero Deretta" <gpderetta-AT-gmail.com> wrote:
If you do not want to deal with move emulation (which I've found very brittle in complex expressions),
Do you mean the old-style move emulation that relied on T(T&) copy ctors (known to be brittle) or does this assessment apply also to the adobe-style emulation (http://stlab.adobe.com/group__move__related.html)? If so, could you explain in more detail?
The former; I have yet to try the latter. I got interested in the boost::move because it supported move only types (which IMHO are the really interesting application of move semantics). Too bad that doesn't seems to be a robust solution in C++0x.
D'oh! I meant C++03!
Move-only types aren't too interesting or difficult to write in C++0x, actually. Have you looked at http://svn.boost.org/trac/boost/browser/trunk/boost/ptr_container/detail/sta... ?
And from a quick glance at that code, I see no rvalue references, so I think you understood what I meant and not what I wrote :). I'll take a deeper look at that code.
a simple way to to gain the advantage of T::operator=(T rhs) even when assigning from lvalues is something like:
template<class T> T destructive_copy(T& x) { using std::swap; T result; swap(result, x); return x; }
This only works when T is both default constructible and has an optimized swap. The latter is detectable, roughly speaking, so the function could be altered to handle it, but the former is not, which it seems to me makes destructive_copy not very useful in generic code.
...
For types which do not have an optimized swap is suboptimal, so some sfinae trick on is_swappable might be needed. Ah, of course it requires T to be DefaultConstructible, and most importantly CopyConstructible so it doesn't handle move only types.
That's a lot of caveats.
I think that the 'adobe move' has the same requirements.
Only because they wanted those requirements anyway.
Yeah, I just read your link to Daniel James simple extension to the adobe move. I'll have to try it.
OTOH, destructive_copy requires no specific support from the 'moved' type: a custom swap and the 'smart' operator= are useful on their own, so adding destructive_copy to one's set of tools requires very little extra baggage.
Very true. For what it's worth, I had been working in this direction to get move optimizations for C++03 when I discussed it with Daniel James a few months ago, and I presume you're aware of this thread: <http://news.gmane.org/find-root.php?message_id=%3cg8hj6n%24oud%241%40ger.gmane.org%3e>.
However, I prefer my assign(x) = y idiom for handling assignment on non-move-or-elision-aware types. See attached. You can detect C++03 move/elision-awareness; combining that with this could be powerful.
It looks nice, expecially for standard containers whose operator= doesn't pass by value. -- gpd
Michael Marcin: ...
For rvalues yes but for lvalues you need move emulation. Which can be implemented in the sandbox move library as follows.
class foo { public: foo(); foo( T t ); foo( boost::move_from<foo> other ) { swap( other.source ); }
foo& operator=( foo rhs ) { swap( rhs ); }
I see what you mean now, thanks. This is, I believe, the Adobe approach for move emulation: http://stlab.adobe.com/group__move__related.html It's based on the Adobe/Stepanov philosophy that all types are copyable, so it doesn't support move-only types. This takes away one motivation for having move. std::vector won't use adobe::move/boost::move, so this takes away the other motivation. What's left? :-)
Peter Dimov wrote:
Michael Marcin: ...
For rvalues yes but for lvalues you need move emulation. Which can be implemented in the sandbox move library as follows.
class foo { public: foo(); foo( T t ); foo( boost::move_from<foo> other ) { swap( other.source ); }
foo& operator=( foo rhs ) { swap( rhs ); }
I see what you mean now, thanks. This is, I believe, the Adobe approach for move emulation:
http://stlab.adobe.com/group__move__related.html
It's based on the Adobe/Stepanov philosophy that all types are copyable, so it doesn't support move-only types. This takes away one motivation for having move. std::vector won't use adobe::move/boost::move, so this takes away the other motivation. What's left? :-)
Adobe may believe that all types are Regular but I have types that can't be copied (at least not efficiently) and I disallow their copying to prevent clients of my code from shooting themselves in the performance foot. The move library in the boost sandbox by Daniel James, which is a continuation of the work by others including Adobe, relaxes these constraints and supports move only types. I use it in some performance sensitive code or when I need/want stronger exception safety guarantees. Thanks, Michael Marcin
on Wed Sep 10 2008, "Peter Dimov" <pdimov-AT-pdimov.com> wrote:
Michael Marcin: ...
For rvalues yes but for lvalues you need move emulation. Which can be implemented in the sandbox move library as follows.
class foo { public: foo(); foo( T t ); foo( boost::move_from<foo> other ) { swap( other.source ); }
foo& operator=( foo rhs ) { swap( rhs ); }
I see what you mean now, thanks. This is, I believe, the Adobe approach for move emulation:
http://stlab.adobe.com/group__move__related.html
It's based on the Adobe/Stepanov philosophy that all types are copyable, so it doesn't support move-only types.
Yes, that's the basis, but adding handling for move-only types is pretty easy (http://news.gmane.org/find-root.php?message_id=%3c31f5f6790806231544h1ec89cd...)
This takes away one motivation for having move. std::vector won't use adobe::move/boost::move, so this takes away the other motivation. What's left? :-)
boost/interprocess/containers/vector.hpp et. al. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
on Tue Sep 09 2008, Niels Dekker - mail address until 2008-12-31 <nd_mail_address_valid_until_2008-12-31-AT-xs4all.nl> wrote:
David Abrahams wrote:
In the case of copy-elision for by-value arguments and return values, the compiler is explicitly allowed to _assume_ there is no semantic difference between the original rvalue and its copy. That's low-hanging fruit for a compiler writer, that pays huge dividends.
So far I've found 7 copy assignment operators that could be improved by having their argument "by value", instead of "by const reference", at the following locations, in trunk/boost:
- any.hpp(64): boost::any - function\function_template.hpp(916): boost::function - intrusive_ptr.hpp(114): intrusive_ptr - multi_index_container.hpp(269): multi_index_container - interprocess\allocators\adaptive_pool.hpp(136): adaptive_pool_base - spirit\home\classic\tree\common.hpp(101): spirit::tree_node - spirit\home\classic\tree\common.hpp(624): spirit::tree_match
Am I correct, or am I possibly missing some others?
Well, I just took a look through boost/iterator/iterator_facade.hpp and found several that could be "conditionally improved" in the case where the rhs has a swap (you probably remember that we discussed detecting that in http://news.gmane.org/find-root.php?message_id=%3c16708896C72A4A51BB534B1187...). I think that also goes for many compiler-generated assignment operators, so arguably your search should include files with no explicit operator=. I suppose this optimization does us no good in base classes, so things like iterator_facade would be exempt, but the specialized adaptors could still benefit from it.
I wouldn't mind creating some tickets (one per source file), requesting to have their argument "by value", as we discussed at comp.lang.c++.moderated, "Re: Is self assignment test valid?".
Might I suggest one per library?
Basically it was concluded that /if/ an assignment operator is implemented by means of copy-and-swap, it should preferably be done as follows:
T& operator=(T arg) { arg.swap(*this); return *this; }
In generic code I strongly prefer T& operator=(T arg) { swap(*this, arg); return *this; } since swap members have little generic value and therefore are only ever implemented as a convenience. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
On Sep 9, 2008, at 6:39 PM, Niels Dekker - mail address until 2008-12-31 wrote:
Basically it was concluded that /if/ an assignment operator is implemented by means of copy-and-swap, it should preferably be done as follows:
T& operator=(T arg) { arg.swap(*this); return *this; }
And preferably /not/ as follows:
T& operator=(const T & arg) { T(arg).swap(*this); return *this; }
Kind regards, -- Niels Dekker http://www.xs4all.nl/~nd/dekkerware Scientific programmer at LKEB, Leiden University Medical Center
For types that take allocators (the ones most likely to benefit from swap) when the allocator follows the (new) scoped allocator model (n2606): if T uses some allocator_type and if std::allocator_propagate_on_copy_assignment<allocator_type> does not derive from std::true_type, then the function swap will do a fast- swap when *this and arg's allocators compare equal (e.g., swap the pointers to data for containers), but will do a slow copy otherwise. In this case, taking the argument by value adds an extra copy, since it is not possible to control the allocator used by the temporary (ideally, we would've liked to use this->get_allocator()). But your msg is is all good: I was going to point out it didn't work in the case described above, but your wording ("/if/ an assignment operator is implemented by means of copy-and-swap") carefully excludes this case, since it is not (always) a copy-and-swap: sometimes it is just a fast swap. So I do agree with you, but developers should probably bear the case I describe above in mind as well... Cheers, -- Hervé Brönnimann hervebronnimann@mac.com
Thanks for all of your feedback so far. I just started by submitting three tickets. Please double-check: #2311: any::operator= should have by-value argument http://svn.boost.org/trac/boost/ticket/2311 #2312: intrusive_ptr::operator= should have by-value argument http://svn.boost.org/trac/boost/ticket/2312 #2313: multi_index_container::operator= should have by-value argument http://svn.boost.org/trac/boost/ticket/2313 Peter Dimov wrote:
/if/ an assignment operator is implemented by means of copy-and-swap, it should preferably be done as follows:
T& operator=(T arg) { arg.swap(*this); return *this; }
This is well known (although the typical form is swap(arg) instead of arg.swap(*this)).
Is the choice between swap(arg) and arg.swap(*this)) just a matter of taste?
It is not widely used in Boost because it creates problems with some compilers (I recall Borland being one). The "classic" form is, unsurprisingly, much better supported across the board. Things may have improved now, of course.
Please let us know if there's a specific compiler version we need to support that wouldn't allow by-value argument for assignment operators. David Abrahams wrote:
In generic code I strongly prefer
T& operator=(T arg) { swap(*this, arg); return *this; }
I'm not sure... It might still compile when T wouldn't have a custom swap. If so, it would possibly pick the /generic/ boost::swap or std::swap, recursively caling the assignment of T and getting into an infinite loop. So it seems to me that its more safe to call the swap /member/ function of T, in this case. To be continued... Niels
Niels Dekker:
Please let us know if there's a specific compiler version we need to support that wouldn't allow by-value argument for assignment operators.
I'm not sure I "get" this "we" vs "you" separation given that it is I who needs to support intrusive_ptr. Anyway, I tried playing with some toy examples and it seems that my "old compiler zoo" is able to handle by-value assignment, so you can disregard my comments as unverified FUD unless confirmed by a regression test breakage.
Niels Dekker:
T& operator=(T arg) { arg.swap(*this); return *this; }
...
Is the choice between swap(arg) and arg.swap(*this)) just a matter of taste?
Yes, I think so. I was just saying that T& operator=( T arg ) { swap( arg ); return *this; } is the form I've seen used (by Dave Abrahams, I think*). It certainly looks more "idiomatic" (in the sense of being recognizable as an idiom as opposed to ordinary code) because of the unusual swap call. *) Turns out that I was right in thinking that. Dave's associative_vector submission: http://lists.boost.org/Archives/boost/2000/05/3200.php contains: Self& operator=(Self rhs) { swap(rhs); return *this; } // strong guarantee I was able to find a post by Valentin Bonnard also suggesting the same: http://tech.groups.yahoo.com/group/boost/message/5001 Incidentally: http://aspn.activestate.com/ASPN/Mail/Message/boost/1140338 "It doesn't compile with Borland C++Builder 4 or BCC 5.5. ... The next problem was associative_vector's assignment operator. Borland didn't recognise it as one and generated a default. Then it couldn't disambiguate the generated assignment operator and the one supplied. I changed this to Self& operator=(const Self &rhs) { Self temp(rhs); swap(temp); return *this; }" My memory is correct on this one, too. I'm not sure why BCC 5.5.1 didn't exhibit the problem in my tests.
I may come late and maybe like an unwanted guest in this discussion but I am curious to know how i can know that for a given class of mine, the swap value operator= is better than the by const ref operator= ? Thanks for the enlightnements
Joel Falcou wrote:
I may come late and maybe like an unwanted guest in this discussion but I am curious to know how i can know that for a given class of mine, the swap value operator= is better than the by const ref operator= ?
My rule of thumb is that if your assignment operator might throw an exception and you want assignment to fulfill the strong exception safety guarantee (i.e. either the assignment succeeds or the target remains unchanged) then you should consider the swapping operator=. Joe Gottman
On Wed, Sep 10, 2008 at 21:23, Joe Gottman <jgottman@carolina.rr.com> wrote:
Joel Falcou wrote:
I may come late and maybe like an unwanted guest in this discussion but I am curious to know how i can know that for a given class of mine, the swap value operator= is better than the by const ref operator= ?
My rule of thumb is that if your assignment operator might throw an exception and you want assignment to fulfill the strong exception safety guarantee (i.e. either the assignment succeeds or the target remains unchanged) then you should consider the swapping operator=.
I would even go slightly more expansive than that and say "Use the swapping operator= unless you've explicitly decided you want less exception safety (like for efficiency reasons in std::vector)". It means one fewer place that needs updating when members change, and I suspect -- though I have no supporting data -- that the optimizer should make the swapping version "good enough" in most cases.
on Wed Sep 10 2008, "Scott McMurray" <me22.ca+boost-AT-gmail.com> wrote:
On Wed, Sep 10, 2008 at 21:23, Joe Gottman <jgottman@carolina.rr.com> wrote:
Joel Falcou wrote:
I may come late and maybe like an unwanted guest in this discussion but I am curious to know how i can know that for a given class of mine, the swap value operator= is better than the by const ref operator= ?
My rule of thumb is that if your assignment operator might throw an exception and you want assignment to fulfill the strong exception safety guarantee (i.e. either the assignment succeeds or the target remains unchanged) then you should consider the swapping operator=.
I would even go slightly more expansive than that and say "Use the swapping operator= unless you've explicitly decided you want less exception safety (like for efficiency reasons in std::vector)".
It means one fewer place that needs updating when members change, and I suspect -- though I have no supporting data -- that the optimizer should make the swapping version "good enough" in most cases.
Swapping and taking the parameter by value are orthogonal issues; the latter has only to do with copy elision and not with exception safety. Please see this thread: http://groups.google.com/group/comp.lang.c++.moderated/browse_frm/thread/97a... -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams a écrit :
Swapping and taking the parameter by value are orthogonal issues; the latter has only to do with copy elision and not with exception safety. Please see this thread: http://groups.google.com/group/comp.lang.c++.moderated/browse_frm/thread/97a...
Very interesting article. So my question is - as a teacher - what techniques should I teach to my student and with which justification ?
on Thu Sep 11 2008, Joel Falcou <joel.falcou-AT-u-psud.fr> wrote:
David Abrahams a écrit :
Swapping and taking the parameter by value are orthogonal issues; the latter has only to do with copy elision and not with exception safety. Please see this thread:
Very interesting article. So my question is - as a teacher - what techniques should I teach to my student and with which justification ?
self& operator=(self x) { swap(*this,x); return *this; } with the rationale given in http://groups.google.com/group/comp.lang.c++.moderated/msg/5e6014446d559b63 (I suspect Andrei's CUJ article is equally good or better but much longer) ...but I would think given the thread mentioned above that answer would be self-evident, so it makes me wonder if you're trying to ask something else. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams a écrit :
self& operator=(self x) { swap(*this,x); return *this; }
with the rationale given in http://groups.google.com/group/comp.lang.c++.moderated/msg/5e6014446d559b63 (I suspect Andrei's CUJ article is equally good or better but much longer)
...but I would think given the thread mentioned above that answer would be self-evident, so it makes me wonder if you're trying to ask something else Sorry if i was unclear. My question was is the old operator= form still needed soemtime , thus making the 2 forms of the operator to be taught.
on Thu Sep 11 2008, Joel Falcou <joel.falcou-AT-u-psud.fr> wrote:
David Abrahams a écrit :
self& operator=(self x) { swap(*this,x); return *this; }
with the rationale given in http://groups.google.com/group/comp.lang.c++.moderated/msg/5e6014446d559b63> (I suspect Andrei's CUJ article is equally good or better but much longer)
...but I would think given the thread mentioned above that answer would be self-evident, so it makes me wonder if you're trying to ask something else Sorry if i was unclear. My question was is the old operator= form still needed soemtime , thus making the 2 forms of the operator to be taught.
The only other form that makes sense for operator= doesn't use swap at all, and does things "the hard way:" destroy LHS resources and then copy the RHS resources into it. You might choose that approach if you were willing to accept the basic guarantee in order to avoid the spike in resource usage caused by having the LHS data and two copies of the RHS data all at once. The standard containers generally do it that way. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
on Wed Sep 10 2008, "Peter Dimov" <pdimov-AT-pdimov.com> wrote:
Niels Dekker:
T& operator=(T arg) { arg.swap(*this); return *this; }
...
Is the choice between swap(arg) and arg.swap(*this)) just a matter of taste?
Yes, I think so. I was just saying that
T& operator=( T arg ) { swap( arg ); return *this; }
is the form I've seen used (by Dave Abrahams, I think*). It certainly looks more "idiomatic" (in the sense of being recognizable as an idiom as opposed to ordinary code) because of the unusual swap call.
*) Turns out that I was right in thinking that. Dave's associative_vector submission:
http://lists.boost.org/Archives/boost/2000/05/3200.php
contains:
Self& operator=(Self rhs) { swap(rhs); return *this; } // strong guarantee
That's interesting. I'm pretty sure that when I wrote that, I wasn't aware of the copy elision advantages and was just trying to write the most compact strong-guarantee assignment that I could.
I was able to find a post by Valentin Bonnard also suggesting the same:
Looks like he wasn't aware of it either
Incidentally:
http://aspn.activestate.com/ASPN/Mail/Message/boost/1140338
"It doesn't compile with Borland C++Builder 4 or BCC 5.5.
Not that I'm worried about those anymore ;-) -- Dave Abrahams BoostPro Computing http://www.boostpro.com
on Wed Sep 10 2008, Niels Dekker - mail address until 2008-12-31 <nd_mail_address_valid_until_2008-12-31-AT-xs4all.nl> wrote:
Thanks for all of your feedback so far. I just started by submitting three tickets. Please double-check:
#2311: any::operator= should have by-value argument http://svn.boost.org/trac/boost/ticket/2311#2312: intrusive_ptr::operator= should have by-value argument http://svn.boost.org/trac/boost/ticket/2312#2313: multi_index_container::operator= should have by-value argument http://svn.boost.org/trac/boost/ticket/2313
Peter Dimov wrote:
/if/ an assignment operator is implemented by means of copy-and-swap, it should preferably be done as follows:
T& operator=(T arg) { arg.swap(*this); return *this; }
This is well known (although the typical form is swap(arg) instead of arg.swap(*this)).
Is the choice between swap(arg) and arg.swap(*this)) just a matter of taste?
It is not widely used in Boost because it creates problems with some compilers (I recall Borland being one). The "classic" form is, unsurprisingly, much better supported across the board. Things may have improved now, of course.
Please let us know if there's a specific compiler version we need to support that wouldn't allow by-value argument for assignment operators.
David Abrahams wrote:
In generic code I strongly prefer
T& operator=(T arg) { swap(*this, arg); return *this; }
I'm not sure... It might still compile when T wouldn't have a custom swap. If so, it would possibly pick the /generic/ boost::swap or std::swap, recursively caling the assignment of T and getting into an infinite loop.
Yuck.
So it seems to me that its more safe to call the swap /member/ function of T, in this case.
Well, you have to know that the member exists in class T (and not in a derived class), just as you have to know that the free function is defined specifically for T, so I'm not sure it's a significant difference. I'm just saying, I guess, that the form using the member forces me to define a swap member even though it's not really needed by generic code. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
In generic code I strongly prefer
T& operator=(T arg) { swap(*this, arg); return *this; }
I'm just saying, I guess, that the form using the member forces me to define a swap member even though it's not really needed by generic code.
I think we should stick to the guideline to always have a swap member function for a class, whenever the class has a custom free swap function. Don't you agree? BTW the swap member function offers a C++03 way to "move assign" an rvalue: std::vector<int> generate_some_data(void); std::vector<int> my_data; // Move the result into my_data: generate_some_data().swap(my_data); Peter Dimov wrote:
Incidentally: http://aspn.activestate.com/ASPN/Mail/Message/boost/1140338 "It doesn't compile with Borland C++Builder 4 or BCC 5.5.
The next problem was associative_vector's assignment operator. Borland didn't recognise it as one and generated a default. Then it couldn't disambiguate the generated assignment operator and the one supplied. [...]
My memory is correct on this one, too. I'm not sure why BCC 5.5.1 didn't exhibit the problem in my tests.
Thanks Peter. Maybe it's a C++Builder 4/BCC 5.5 bug that was fixed by BCC 5.5.1. :-) Anyway, neither of them are still at the regression pages. Siliconman runs the tests on BCC 5.6.4, 5.8.2, and 5.9.3, while JustSoftwareSolutions does BCC 5.8.2. Thanks Joaquín, for already applying the patch from ticket #2313 to multi_index_container::operator= Things still look fine at the regression page <www.boost.org/development/tests/trunk/developer/multi_index.html> but I guess we have to wait a few days longer... Kind regards, Niels
On Thu, Sep 11, 2008 at 11:07 AM, Niels Dekker - mail address until 2008-12-31 <nd_mail_address_valid_until_2008-12-31@xs4all.nl> wrote:
David Abrahams wrote:
In generic code I strongly prefer
T& operator=(T arg) { swap(*this, arg); return *this; }
I'm just saying, I guess, that the form using the member forces me to define a swap member even though it's not really needed by generic code.
I think we should stick to the guideline to always have a swap member function for a class, whenever the class has a custom free swap function. Don't you agree?
BTW the swap member function offers a C++03 way to "move assign" an rvalue:
std::vector<int> generate_some_data(void); std::vector<int> my_data; // Move the result into my_data: generate_some_data().swap(my_data);
I guess this is a common idiom, I use it a lot. OTOH if my_data::operator=() used pass by value (which in the case of standard containers is, unfortunately, not true), the member swap wouldn't be necessary. In practice this means that, for your own classes, or you implement the 'smart' operator= or you need the member swap (to swap out of temporaries). You do not need both. -- gpd
BTW the swap member function offers a C++03 way to "move assign" [from] an rvalue:
std::vector<int> generate_some_data(void); std::vector<int> my_data; // Move the result into my_data: generate_some_data().swap(my_data);
Giovanni Piero Deretta wrote:
I guess this is a common idiom, I use it a lot.
OTOH if my_data::operator=() used pass by value (which in the case of standard containers is, unfortunately, not true), the member swap wouldn't be necessary.
In practice this means that, for your own classes, or you implement the 'smart' operator= or you need the member swap (to swap out of temporaries). You do not need both.
When having a by-value argument for operator=, a compiler is /allowed/ to do copy elision, but it's not /required/ to do do. So I'd recommend you to just keep swapping your rvalues (using member swap) if you already do so anyway :-) Kind regards, Niels
on Thu Sep 11 2008, "Niels Dekker - mail address until 2008-12-31" <nd_mail_address_valid_until_2008-12-31-AT-xs4all.nl> wrote:
BTW the swap member function offers a C++03 way to "move assign" [from] an rvalue:
std::vector<int> generate_some_data(void); std::vector<int> my_data; // Move the result into my_data: generate_some_data().swap(my_data);
Giovanni Piero Deretta wrote:
I guess this is a common idiom, I use it a lot.
OTOH if my_data::operator=() used pass by value (which in the case of standard containers is, unfortunately, not true), the member swap wouldn't be necessary.
In practice this means that, for your own classes, or you implement the 'smart' operator= or you need the member swap (to swap out of temporaries). You do not need both.
When having a by-value argument for operator=, a compiler is /allowed/ to do copy elision, but it's not /required/ to do do. So I'd recommend you to just keep swapping your rvalues (using member swap) if you already do so anyway :-)
The downside is that your code remains ugly even when you'll get automatic optimization from the compiler for the clear syntax (or in C++0x, move semantics support). -- Dave Abrahams BoostPro Computing http://www.boostpro.com
on Thu Sep 11 2008, "Niels Dekker - mail address until 2008-12-31" <nd_mail_address_valid_until_2008-12-31-AT-xs4all.nl> wrote:
David Abrahams wrote:
In generic code I strongly prefer
T& operator=(T arg) { swap(*this, arg); return *this; }
I'm just saying, I guess, that the form using the member forces me to define a swap member even though it's not really needed by generic code.
I think we should stick to the guideline to always have a swap member function for a class, whenever the class has a custom free swap function. Don't you agree?
Sorry, but no, I just don't see the point. It feels like needless clutter. Generic code needs a free function that can be found via ADL, so I would normally make it a friend declared inside the class body. There's nothing wrong with adding a swap member, but I'd be loathe to make it a guideline.
BTW the swap member function offers a C++03 way to "move assign" an rvalue:
std::vector<int> generate_some_data(void); std::vector<int> my_data; // Move the result into my_data: generate_some_data().swap(my_data);
Well, that's true, but I much prefer: assign(my_data) = generate_some_data(); because it better reflects the logical operation.
Peter Dimov wrote:
Incidentally: http://aspn.activestate.com/ASPN/Mail/Message/boost/1140338> "It doesn't compile with Borland C++Builder 4 or BCC 5.5.
The next problem was associative_vector's assignment operator. Borland didn't recognise it as one and generated a default. Then it couldn't disambiguate the generated assignment operator and the one supplied. [...]
My memory is correct on this one, too. I'm not sure why BCC 5.5.1 didn't exhibit the problem in my tests.
Thanks Peter. Maybe it's a C++Builder 4/BCC 5.5 bug that was fixed by BCC 5.5.1. :-) Anyway, neither of them are still at the regression pages. Siliconman runs the tests on BCC 5.6.4, 5.8.2, and 5.9.3, while JustSoftwareSolutions does BCC 5.8.2.
Thanks Joaquín, for already applying the patch from ticket #2313 to multi_index_container::operator= Things still look fine at the regression page <www.boost.org/development/tests/trunk/developer/multi_index.html> but I guess we have to wait a few days longer...
Niels: are you planning to pursue tickets for the classes with implicit assignment operators and the others I mentioned in response to your original post? -- Dave Abrahams BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
Niels: are you planning to pursue tickets for the classes with implicit assignment operators and the others I mentioned in response to your original post?
Honestly I haven't yet looked into those. But my idea was to start off by submitting tickets to get that "low-hanging fruit", for those cases that just need a few small changes in their existing assignment operators. I'd rather wait and see if those simple cases are going well (especially at the regression site). But feel free to create tickets for those implicit assignment operators if you can't wait any longer. :-) I've just submitted a ticket to Douglas, for boost::function: https://svn.boost.org/trac/boost/ticket/2319 boost::function's copy assignment operator is kinda interesting, because it currently does copy-and-swap, while it still doesn't provide the strong guarantee. The "function_assignment.patch" that I attached to #2319 should significantly reduce the chance of having the assignment fail, but it still won't provide the strong guarantee. I'm afraid that's the price of small-object optimization. Do you think that it would be okay in C++0X to have operator= overloaded for by-value (copy-assignment) and by-rvalue-reference (move-assignment)? Or would it be preferable in C++0X to have a const-reference as copy-assignment argument? Kind regards, Niels
Niels Dekker - mail address until 2008-12-31 wrote:
David Abrahams wrote:
In the case of copy-elision for by-value arguments and return values, the compiler is explicitly allowed to _assume_ there is no semantic difference between the original rvalue and its copy. That's low-hanging fruit for a compiler writer, that pays huge dividends.
Basically it was concluded that /if/ an assignment operator is implemented by means of copy-and-swap, it should preferably be done as follows:
T& operator=(T arg) { arg.swap(*this); return *this; }
And preferably /not/ as follows:
T& operator=(const T & arg) { T(arg).swap(*this); return *this; }
One downside is that self-assignment is made suboptimal. My typical operator= implementation is: T& operator= (T const& that) { if (this != &that) T(that).swap(*this); return *this; } Not sure, however, if it is critical enough.
Andrey Semashev wrote:
One downside is that self-assignment is made suboptimal. My typical operator= implementation is:
T& operator= (T const& that) { if (this != &that) T(that).swap(*this); return *this; }
Not sure, however, if it is critical enough.
In practice I haven't written any interesting code that does self assignment. Perhaps you have. In my code I'd consider this a pessimization. Thanks, Michael Marcin
on Wed Sep 10 2008, Andrey Semashev <andrey.semashev-AT-gmail.com> wrote:
One downside is that self-assignment is made suboptimal. My typical operator= implementation is:
T& operator= (T const& that) { if (this != &that) T(that).swap(*this); return *this; }
Not sure, however, if it is critical enough.
IMO it is not only non-critical, but the extra little bit of complexity involved in worrying about self-assignment is slightly dangerous, and it pessimizes the usual case (non-self-assignment) by adding a test and branch. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
participants (10)
-
Andrey Semashev -
David Abrahams -
Giovanni Piero Deretta -
Hervé Brönnimann -
Joe Gottman -
Joel Falcou -
Michael Marcin -
Niels Dekker - mail address until 2008-12-31 -
Peter Dimov -
Scott McMurray