I just discovered that optional<T> is not safe under self-assignment. Consider the following code fragment: boost::optional<long> x(1); //At this point x is initialized and *x == 1 x = x; //x should be unchanged, but instead x is now uninitialized Optional's assignment operator looks like optional& operator= ( optional const& rhs ) { this->assign( rhs ) ; return *this ; } and assign is defined by void assign ( argument_type val ) { destroy(); construct(val); } Thus, if (this == &rhs) in operator=(), any value held by rhs will be destroyed. I can see two possible fixes for this. the first is to put an "if (this != &rhs) " around the call to assign in operator=(). This is simplest, but it slows the code down in order to protect against a very rare situation. The other is to rewrite assign as follows: void assign(argument_type val) { if (m_initialized) { get() = val; // call assignment operator } else { construct(val); //call copy constructor } return *this; } This makes optional self-assignment safe. It has the added advantage that is T's assignment operator has the strong exception safety guarantee, so does optional<T>'s assignment operator. It has the disadvantage of making optional depend on both assignment operator and copy constructor to copy, but very few classes have a copy constructor but don't have an assignment operator. Joe Gottman
On Mon, 14 Feb 2005 20:35:31 -0500 "Joe Gottman" <jgottman@carolina.rr.com> wrote:
I just discovered that optional<T> is not safe under self-assignment. Consider the following code fragment:
boost::optional<long> x(1); //At this point x is initialized and *x == 1 x = x; //x should be unchanged, but instead x is now uninitialized
Optional's assignment operator looks like
optional& operator= ( optional const& rhs ) { this->assign( rhs ) ; return *this ; }
and assign is defined by
void assign ( argument_type val ) { destroy(); construct(val); }
Thus, if (this == &rhs) in operator=(), any value held by rhs will be destroyed.
Actually, there are multiple assignment operators, with different levels of complexity and "bugginess." It seems to me that implementing the canonical assignment operator would work fine... optional & operator=(optional const & rhs) { optional tmp(rhs); optional_swap(*this, tmp); return *this; } I know it is has a little more overhead, but I am pretty sure there are enough copy-ctors to match the assignement signatures that each assignemnt operator could be implemented in this manner fairly simply. This actually brings me to a more broad question... with a fairly well accepted practice for safe implementation of assignment operators, some boost libs seem to go out of their way to implement something different. Is this because of perceived performance issues, or something else?
"Jody Hagins" <jody-boost-011304@atdesk.com> escribió en el mensaje news:20050215103732.03af02c0.jody-boost-011304@atdesk.com...
Actually, there are multiple assignment operators, with different levels of complexity and "bugginess."
Care to detail these levels of "bugginess"? Only one of the assignment overloads suffers from the self-assignment bug anyway.
It seems to me that implementing the canonical assignment operator would work fine...
optional & operator=(optional const & rhs) { optional tmp(rhs); optional_swap(*this, tmp); return *this; }
The purpose of this idiom is provide a better exception guarantee based on a safe swap (preferably a non throw swap). If the swap is no better (exception-safety-wise) than a straight assignment, the idiom is unnecesary. It turns out that optional_swap gives only the basic guarantee, and so does assign(), so there is no point in this.
This actually brings me to a more broad question... with a fairly well accepted practice for safe implementation of assignment operators, some boost libs seem to go out of their way to implement something different. Is this because of perceived performance issues, or something else?
Something else: purpose. The right idiom in the wrong place (or context) is a Bad Thing. Best Fernando Cacciola
Jody Hagins <jody-boost-011304@atdesk.com> writes:
It seems to me that implementing the canonical assignment operator would work fine...
optional & operator=(optional const & rhs) { optional tmp(rhs); optional_swap(*this, tmp); return *this; }
I wish people would stop calling it that. It's easy, but it's overkill.
I know it is has a little more overhead
Yes. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
Jody Hagins <jody-boost-011304@atdesk.com> writes:
It seems to me that implementing the canonical assignment operator would work fine...
optional & operator=(optional const & rhs) { optional tmp(rhs); optional_swap(*this, tmp); return *this; }
I wish people would stop calling it that. It's easy, but it's overkill.
No, it is not overkill. It is the easiest way to implement a correct assignment operator. Forget performance. Subtle bugs in assignment are notoriously hard to find. To give the "canonical" example: optional& operator=( T const & t ) { // destroy and recreate } Consider what happens when an optional holds a tree node and is assigned a child of that node. Or in general, when the 'destroy' step indirectly destroys the right hand side. A correct assignment operator should typically handle self-assignment without an explicit test. Adding the test usually doesn't fix the problem, only its most obvious manifestation. Of course if correctness is not a concern, copy+swap is always overkill.
On Tue, 15 Feb 2005 21:33:59 +0200, Peter Dimov <pdimov@mmltd.net> wrote:
David Abrahams wrote:
Jody Hagins <jody-boost-011304@atdesk.com> writes:
It seems to me that implementing the canonical assignment operator would work fine...
optional & operator=(optional const & rhs) { optional tmp(rhs); optional_swap(*this, tmp); return *this; }
Why not: optional& operator=(optional rhs) { optional_swap(*this, rhs); return *this; } ? Bruno
On Tue, 15 Feb 2005 18:02:57 -0200 Bruno Martínez Aguerre <br1@internet.com.uy> wrote:
Why not:
optional& operator=(optional rhs) { optional_swap(*this, rhs); return *this; } ?
That is a very valid option as well. In fact, in his book, Sutter mentions that option specifically... Exceptional C++, page 48: "If you're one of those folks who like terse code, you can write the operator=() canonical form more compactly as: Stack& operator=(Stack other) { Swap( other ); return *this; } " Not being very up to date on optimization techniques, I do not know if a compiler can turn one into better code than the other. Barring that, I prefer the less-terse one, because I also prefer passing ref-to-const over by-value, unless there is a good reason to do otherwise.
Jody Hagins wrote:
That is a very valid option as well. In fact, in his book, Sutter mentions that option specifically...
Exceptional C++, page 48:
"If you're one of those folks who like terse code, you can write the operator=() canonical form more compactly as:
Stack& operator=(Stack other) { Swap( other ); return *this; } "
Not being very up to date on optimization techniques, I do not know if a compiler can turn one into better code than the other.
Yes, it can. Consider the case where the definition of operator= is not visible to the compiler and one does s = Stack(); With the by-value version the temporary Stack() will be constructed directly in 'other', saving a copy. Of course in a future language with rvalue references the "Stack&& other" overload will be used instead, with similar or better performance. http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2004/n1690.html
"Peter Dimov" <pdimov@mmltd.net> escribió en el mensaje news:003201c51395$4f587820$6501a8c0@pdimov2...
David Abrahams wrote:
Jody Hagins <jody-boost-011304@atdesk.com> writes:
It seems to me that implementing the canonical assignment operator would work fine...
optional & operator=(optional const & rhs) { optional tmp(rhs); optional_swap(*this, tmp); return *this; }
I wish people would stop calling it that. It's easy, but it's overkill.
No, it is not overkill. It is the easiest way to implement a correct assignment operator. Forget performance. Subtle bugs in assignment are notoriously hard to find. To give the "canonical" example:
optional& operator=( T const & t ) { // destroy and recreate }
Consider what happens when an optional holds a tree node and is assigned a child of that node. Or in general, when the 'destroy' step indirectly destroys the right hand side.
Well, I would be surprise if T were so unlike a POD to have this sort of issues... but well, who knows. I would recommend shared_ptr<> in that case, but, well, maybe genericity get's in the way. I just looked closer at optional_swap() and it offers no exception-safety guarantees at all... It just does whatever T's swap() does. operator=() was designed to give a basic guarantee in spite of T. Thus, as I replied to Joe, changing the implementation means loosing the basic guarantee and leave it up entirely to T... I'm not sure how much good or bad that is... but I do remember that it was designed like that on purpose after long discussions here.. or at least that's what I concluded from those discussions ;)
A correct assignment operator should typically handle self-assignment without an explicit test. Adding the test usually doesn't fix the problem, only its most obvious manifestation.
Which could be accounted as good enough for classes intended to be used in certain ways, thus making the swap idiom overkill. Fernando Cacciola
Fernando Cacciola wrote:
"Peter Dimov" <pdimov@mmltd.net> escribió en el mensaje news:003201c51395$4f587820$6501a8c0@pdimov2...
A correct assignment operator should typically handle self-assignment without an explicit test. Adding the test usually doesn't fix the problem, only its most obvious manifestation.
Which could be accounted as good enough for classes intended to be used in certain ways, thus making the swap idiom overkill.
It would (probably) be OK to offer a "limited" assignment operator that doesn't handle certain corner cases, but only if these cases are clearly documented as part of the preconditions of operator=. But I'm really not sure why should optional<> attempt to provide a basic guarantee when the contained type does not (unless the basic guarantee is free, which it isn't.) A type that doesn't provide the basic guarantee is pretty much unusable. It seems to me that the straightforward solution is to just defer to T::operator= when necessary. Those that want destroy+recreate semantics can always obtain them explicitly.
On Tue, 15 Feb 2005 13:54:27 -0500 David Abrahams <dave@boost-consulting.com> wrote:
I wish people would stop calling it that. It's easy, but it's overkill.
I got the term from "Exceptional C++" where Herb Sutter calls it "the operator()= canonical form." I am certainly open to alternatives (especially since I didn't invent the term ;-).
I know it is has a little more overhead
Yes.
Sure. The same could be said of shared_ptr and many other useful, well-established, tools and techniques that help the programmer write more correct code. IMO, this "operator=() canonical form" is by far the best initial solution for any non-trivial class. Make sure your copy-ctor and swap do the right thing (which you have to do anyway), and you are set. It is simple, elegant, and in most cases has fairly small overhead. It also helps emphasize correctness first. If the performance becomes a noticable problem, then attack it, but by then you will already have several tests for correctness and performance, which means that any problems in the changed implementation will be detected quickly. As always, a comment about the performance needs of the code is nice, as it prevents others from changing the performance characteristics later.
Jody Hagins <jody-boost-011304@atdesk.com> writes:
Sure. The same could be said of shared_ptr and many other useful, well-established, tools and techniques that help the programmer write more correct code.
IMO, this "operator=() canonical form" is by far the best initial solution for any non-trivial class. Make sure your copy-ctor and swap do the right thing (which you have to do anyway), and you are set. It is simple, elegant, and in most cases has fairly small overhead. It also helps emphasize correctness first.
If the performance becomes a noticable problem, then attack it, but by then you will already have several tests for correctness and performance, which means that any problems in the changed implementation will be detected quickly. As always, a comment about the performance needs of the code is nice, as it prevents others from changing the performance characteristics later.
Okay, okay. I guess I am overly defensive about this because when I "invented" the technique (**) and brought it to peoples' attention as a way to get the strong guarantee for any mutating operation, it seemed that the next conclusion many people (outside the commitee) drew was strong guarantee == good basic guarantee == almost useless And the logical follow-on conclusion among many was It's worth doing almost anything to get the strong guarantee. So I've been trying to shake that idea loose for a long time. But, yeah, as a rule, correctness is more important than performance, and the copy-and-swap formula is a great way to get a correct assignment operator, and to avoid code duplication to boot. (**) I'm _sure_ other people have thought of this technique, so I'm not really trying to claim credit. More like the opposite. The fact remains that it seemed like news to the committee when I brought it up in 1997 as I was trying to get exception-safety into the standard. -- Dave Abrahams Boost Consulting www.boost-consulting.com
"Joe Gottman" <jgottman@carolina.rr.com> escribió en el mensaje news:curjg1$2vq$1@sea.gmane.org...
I just discovered that optional<T> is not safe under self-assignment.
Ouch!! This is a major oversight from my part... Every C++ 101 book warns about it
Consider the following code fragment:
boost::optional<long> x(1); //At this point x is initialized and *x == 1 x = x; //x should be unchanged, but instead x is now uninitialized
Optional's assignment operator looks like
optional& operator= ( optional const& rhs ) { this->assign( rhs ) ; return *this ; }
and assign is defined by
void assign ( argument_type val ) { destroy(); construct(val); }
Well, this is not the correct overload, but you are right all the same.
Thus, if (this == &rhs) in operator=(), any value held by rhs will be destroyed.
Indeed (what was I thinking!)
I can see two possible fixes for this. the first is to put an "if (this != &rhs) " around the call to assign in operator=(). This is simplest, but it slows the code down in order to protect against a very rare situation. The other is to rewrite assign as follows:
void assign(argument_type val) { if (m_initialized) { get() = val; // call assignment operator } else { construct(val); //call copy constructor } return *this; }
This makes optional self-assignment safe.
Right.
It has the added advantage that if T's assignment operator has the strong exception safety guarantee, so does optional<T>'s assignment operator.
Well, this is a good point. It is typical for classes with a throwing copy ctor to have a strong assignment.
It has the disadvantage of making optional depend on both assignment operator and copy constructor to copy, but very few classes have a copy constructor but don't have an assignment operator.
Right, I don't think this extra requirement will have any problems in practice. Thanks for the report! Fernando Cacciola
"Fernando Cacciola" <fernando_cacciola@hotmail.com> escribió en el mensaje news:cutcrj$tcb$1@sea.gmane.org...
"Joe Gottman" <jgottman@carolina.rr.com> escribió en el mensaje news:curjg1$2vq$1@sea.gmane.org...
[SNIP]
It has the added advantage that if T's assignment operator has the strong exception safety guarantee, so does optional<T>'s assignment operator.
Well, this is a good point. It is typical for classes with a throwing copy ctor to have a strong assignment.
OK, I was sure there was a good reason why I didn't use T's assignment to begin with. I just didn't remeber it when I responded. The idea is that by using T's operator=() we are _entirely_ left to its own exception guarantees, with no guarantee at all from optional<T> itself. The current implementation gives you the basic guarantee, which is more than enough in most cases, no matter how ill T could be: if something goes wrong while reseting the value of *this, it is left uninitialized. So the correct path is to add the cannonical guard AFAICS. Cheers Fernando Cacciola
"Fernando Cacciola" <fernando_cacciola@hotmail.com> wrote in message news:cute52$2g5$1@sea.gmane.org...
"Fernando Cacciola" <fernando_cacciola@hotmail.com> escribió en el mensaje news:cutcrj$tcb$1@sea.gmane.org...
"Joe Gottman" <jgottman@carolina.rr.com> escribió en el mensaje news:curjg1$2vq$1@sea.gmane.org...
[SNIP]
It has the added advantage that if T's assignment operator has the strong exception safety guarantee, so does optional<T>'s assignment operator.
Well, this is a good point. It is typical for classes with a throwing copy ctor to have a strong assignment.
OK, I was sure there was a good reason why I didn't use T's assignment to begin with. I just didn't remeber it when I responded. The idea is that by using T's operator=() we are _entirely_ left to its own exception guarantees, with no guarantee at all from optional<T> itself. The current implementation gives you the basic guarantee, which is more than enough in most cases, no matter how ill T could be: if something goes wrong while reseting the value of *this, it is left uninitialized.
So the correct path is to add the cannonical guard AFAICS.
I disagree. This may ensure the basic exception-safety guarantee, but most code that uses optional<T> will use T::operator=() at some point or other. So if T::operator=() is not exception-safe, chances are good any program that uses optional<T> will not be exception safe either, regardless of whether optional<T> is. Thus, you are not gaining too much. And you are losing the strong exception safety guarantee when T::operator=() has it. I think you are much better off implementing optional<T>'s assignment from T's rather than using the destroy and recreate idiom. Joe Gottman
On Tue, 15 Feb 2005 20:35:30 -0500, Joe Gottman <jgottman@carolina.rr.com> wrote: I disagree. This may ensure the basic exception-safety guarantee, but most code that uses optional<T> will use T::operator=() at some point or other. So if T::operator=() is not exception-safe, chances are good any program that uses optional<T> will not be exception safe either, regardless of whether optional<T> is. Thus, you are not gaining too much. And you are losing the strong exception safety guarantee when T::operator=() has it. I think you are much better off implementing optional<T>'s assignment from T's rather than using the destroy and recreate idiom.
Interesting debate. Performance / safety tradeoffs. You shouldn't pay for what you don't use issues. I wonder if anyone can point me to priors on a PBD approach to exception safety: e.g. boost::optional<thing, nutz::strong_guarantee > matt. matthurd@acm.org
"Joe Gottman" <jgottman@carolina.rr.com> writes:
I disagree. This may ensure the basic exception-safety guarantee, but most code that uses optional<T> will use T::operator=() at some point or other. So if T::operator=() is not exception-safe
Whoa, here we go again! You seem to be assuming basic exception-safety guarantee == "not exception-safe." That's just not true for any reasonable definition of "exception-safe." Or am I misunderstanding you? -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
"Joe Gottman" <jgottman@carolina.rr.com> writes:
I disagree. This may ensure the basic exception-safety guarantee, but most code that uses optional<T> will use T::operator=() at some point or other. So if T::operator=() is not exception-safe
Whoa, here we go again! You seem to be assuming
basic exception-safety guarantee == "not exception-safe."
That's just not true for any reasonable definition of "exception-safe."
No, he isn't assuming that. "Not exception safe" == "does not provide basic" in the above paragraph. Fernando's goal, IIUC, was to make optional<T>::operator= provide the basic guarantee even when T::operator= does not. We are arguing that this is not necessary and that optional<T>::operator= should just mirror the exception safety guarantee of T::operator= and T::T( T const & ).
On Wed, 16 Feb 2005 15:29:37 +0200 "Peter Dimov" <pdimov@mmltd.net> wrote:
No, he isn't assuming that. "Not exception safe" == "does not provide basic" in the above paragraph. Fernando's goal, IIUC, was to make optional<T>::operator= provide the basic guarantee even when T::operator= does not. We are arguing that this is not necessary and that optional<T>::operator= should just mirror the exception safety guarantee of T::operator= and T::T( T const & ).
That is how I read it as well, and I also agree that providing more exception safety than T is unwarranted for Boost.Optional. Why should the "optional" nature of T make it more exception safe than using T by itself? The only thing I can think of specific to Boost.Optional is when T is destroyed so that the "optional" nature is "cleared." However, ~T() throws then the programmer has more then enough other problems...
"Peter Dimov" <pdimov@mmltd.net> writes:
David Abrahams wrote:
"Joe Gottman" <jgottman@carolina.rr.com> writes:
I disagree. This may ensure the basic exception-safety guarantee, but most code that uses optional<T> will use T::operator=() at some point or other. So if T::operator=() is not exception-safe
Whoa, here we go again! You seem to be assuming
basic exception-safety guarantee == "not exception-safe."
That's just not true for any reasonable definition of "exception-safe."
No, he isn't assuming that. "Not exception safe" == "does not provide basic" in the above paragraph. Fernando's goal, IIUC, was to make optional<T>::operator= provide the basic guarantee even when T::operator= does not. We are arguing that this is not necessary
Agreed. There's no good reason to try to "fix" broken classes.
and that optional<T>::operator= should just mirror the exception safety guarantee of T::operator= and T::T( T const & ).
Sounds fine to me. -- Dave Abrahams Boost Consulting www.boost-consulting.com
"David Abrahams" <dave@boost-consulting.com> escribió en el mensaje news:u3bvwtvvq.fsf@boost-consulting.com...
"Peter Dimov" <pdimov@mmltd.net> writes:
David Abrahams wrote:
"Joe Gottman" <jgottman@carolina.rr.com> writes:
I disagree. This may ensure the basic exception-safety guarantee, but most code that uses optional<T> will use T::operator=() at some point or other. So if T::operator=() is not exception-safe
Whoa, here we go again! You seem to be assuming
basic exception-safety guarantee == "not exception-safe."
That's just not true for any reasonable definition of "exception-safe."
No, he isn't assuming that. "Not exception safe" == "does not provide basic" in the above paragraph. Fernando's goal, IIUC, was to make optional<T>::operator= provide the basic guarantee even when T::operator= does not. We are arguing that this is not necessary
Agreed. There's no good reason to try to "fix" broken classes.
and that optional<T>::operator= should just mirror the exception safety guarantee of T::operator= and T::T( T const & ).
Sounds fine to me.
Well, I'm kind of sure that this was discussed before when Boost.Optional was being reviewed and the conclusions were the opposite... but maybe I just got it wrong. I have to admit that on a fresh look at it I can only agree with Peter here regarding the unwarranted additional guarantees from Optional. That being said, I'm still unsure that the overhead of the swap idiom is worth it in this case were the swap isn't no-throw (that is, given that the idiom does not provide by itself any additional guarantees) The correct fix along Joe's initial proposal would be: void assign(optional_base const& rhs) { if (is_initialized) { if ( rhs.is_initialized ) get_impl() = rhs.get_impl(); else destroy(); } else { if ( rhs.is_initialized ) construct(rhs.get_impl()); } } AFAICS, this code is as safe aliasing-wise as it can be (it handles not only the trivial case of this==&rhs but also any other deeper aliasing issues) If no voice is raised I'll proceed with this fix. Best, Fernando Cacciola
On Wed, 16 Feb 2005 12:10:01 -0300 "Fernando Cacciola" <fernando_cacciola@hotmail.com> wrote:
The correct fix along Joe's initial proposal would be:
void assign(optional_base const& rhs) { if (is_initialized) { if ( rhs.is_initialized ) get_impl() = rhs.get_impl(); else destroy(); } else { if ( rhs.is_initialized ) construct(rhs.get_impl()); } }
AFAICS, this code is as safe aliasing-wise as it can be (it handles not only the trivial case of this==&rhs but also any other deeper aliasing issues)
If no voice is raised I'll proceed with this fix.
Unfortunately, I believe you now have a new dependency on copy-assignment (i.e., T::operator=()), which IIRC, did not previously exist. This may break existing code for users whose classes do not implement copy assignment.
"Jody Hagins" <jody-boost-011304@atdesk.com> escribió en el mensaje news:20050216142546.4f00c668.jody-boost-011304@atdesk.com...
On Wed, 16 Feb 2005 12:10:01 -0300 "Fernando Cacciola" <fernando_cacciola@hotmail.com> wrote:
The correct fix along Joe's initial proposal would be:
void assign(optional_base const& rhs) { if (is_initialized) { if ( rhs.is_initialized ) get_impl() = rhs.get_impl(); else destroy(); } else { if ( rhs.is_initialized ) construct(rhs.get_impl()); } }
AFAICS, this code is as safe aliasing-wise as it can be (it handles not only the trivial case of this==&rhs but also any other deeper aliasing issues)
If no voice is raised I'll proceed with this fix.
Unfortunately, I believe you now have a new dependency on copy-assignment (i.e., T::operator=()), which IIRC, did not previously exist. This may break existing code for users whose classes do not implement copy assignment.
Yes I now. Joe did when he first proposed the fix, and I think that Peter and David knows this too. Yet, I (we?) don't think there would be any real user code breaking becasue of this... Or to put it another way, if a class is so broken as to support a reasonable copy-constructor (which is a current requirement), but not assignment, then I'm probably doing him/her a favor by breaking it :-) Consider that an assignment operator is such a fundamental thing that the compiler generates one for you if you don't. IMO, there are no classes with no support for assignment.. at most, they have the wrong user-defined assignment. If the semantics of T's assignment are in any way odd (say auto_ptr), then that of optional<T> should really be just as equally odd. Best Fernando Cacciola
"Fernando Cacciola" <fernando_cacciola@hotmail.com> writes:
Or to put it another way, if a class is so broken as to support a reasonable copy-constructor (which is a current requirement), but not assignment, then I'm probably doing him/her a favor by breaking it :-)
?? Any immutable or const type should fit that description. -- Dave Abrahams Boost Consulting www.boost-consulting.com
"David Abrahams" <dave@boost-consulting.com> escribió en el mensaje news:umzu3t5yn.fsf@boost-consulting.com...
"Fernando Cacciola" <fernando_cacciola@hotmail.com> writes:
Or to put it another way, if a class is so broken as to support a reasonable copy-constructor (which is a current requirement), but not assignment, then I'm probably doing him/her a favor by breaking it :-)
?? Any immutable or const type should fit that description.
A const type is not a class. A class by itself would support proper assignment even if you can't use it on a const lvalue. IOW, if you have a proper copy ctor you must have a proper assignment operator. Inmutability given by type qualification is something else... Or are you referring to classes which do implement operator=() but purposedly to do something semantically different than to yield two equivalent objects in the exact same way a copy-ctor will produce them? Fernando Cacciola
"Fernando Cacciola" <fernando_cacciola@hotmail.com> writes:
"David Abrahams" <dave@boost-consulting.com> escribió en el mensaje news:umzu3t5yn.fsf@boost-consulting.com...
"Fernando Cacciola" <fernando_cacciola@hotmail.com> writes:
Or to put it another way, if a class is so broken as to support a reasonable copy-constructor (which is a current requirement), but not assignment, then I'm probably doing him/her a favor by breaking it :-)
?? Any immutable or const type should fit that description.
A const type is not a class. A class by itself would support proper assignment even if you can't use it on a const lvalue.
IOW, if you have a proper copy ctor you must have a proper assignment operator.
I don't follow. I can build a type that can't be mutated through assignment, yet can be copied. It seems like a thoroughly coherent design to me. You're saying it's invalid?
Or are you referring to classes which do implement operator=() but purposedly to do something semantically different than to yield two equivalent objects in the exact same way a copy-ctor will produce them?
No; that _is_ abominable. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
"Fernando Cacciola" <fernando_cacciola@hotmail.com> writes:
"David Abrahams" <dave@boost-consulting.com> escribió en el mensaje news:umzu3t5yn.fsf@boost-consulting.com...
"Fernando Cacciola" <fernando_cacciola@hotmail.com> writes:
Or to put it another way, if a class is so broken as to support a reasonable copy-constructor (which is a current requirement), but not assignment, then I'm probably doing him/her a favor by breaking it :-)
?? Any immutable or const type should fit that description.
A const type is not a class. A class by itself would support proper assignment even if you can't use it on a const lvalue.
IOW, if you have a proper copy ctor you must have a proper assignment operator.
I don't follow. I can build a type that can't be mutated through assignment, yet can be copied. It seems like a thoroughly coherent design to me. You're saying it's invalid?
Ha, Ok, I got your point wrong.. you mean classes that just won't compile if operator=() is used. With: "if it has copy but no assigment then I better break that code" I meant the optional<> code that is syntactically (thus conceptually) doing assignment on a T class that explicitely disallow that. Since the current implementation uses the copy-ctor under the hood, the following can happen now: T a ; T b ; b = a ; // forbidden, even though T b(a) is OK optional<T> oa ; optional<T> ob ; oa = ob ; // OK, but it shouldn't. IMO this is really a bug in optional<T> because if you can't do "a=b" you shouldn't be able to do "oa=ob". So my change will let users uncover such problems (that's what I meant with I better break that code) After all, the old code won't compile anymore, so it won't be a catastrophic break as the one that comes from a change in semantics. Now, that is my personal opinion. You have the experience of the evolution of the std library, so tell me: Is introducing a compiler error in old code that used to compile fine (but do the wrong thing) a good idea? Is this acceptable? Fernando Cacciola
"Fernando Cacciola" <fernando_cacciola@hotmail.com> writes:
You have the experience of the evolution of the std library, so tell me: Is introducing a compiler error in old code that used to compile fine (but do the wrong thing) a good idea? Is this acceptable?
My experiences don't give me any opinion on these questions, in general. -- Dave Abrahams Boost Consulting www.boost-consulting.com
"Fernando Cacciola" <fernando_cacciola@hotmail.com> wrote in message news:cuvnj8$urr$1@sea.gmane.org...
The correct fix along Joe's initial proposal would be:
void assign(optional_base const& rhs) { if (is_initialized) { if ( rhs.is_initialized ) get_impl() = rhs.get_impl(); else destroy(); } else { if ( rhs.is_initialized ) construct(rhs.get_impl()); } }
AFAICS, this code is as safe aliasing-wise as it can be (it handles not only the trivial case of this==&rhs but also any other deeper aliasing issues)
If no voice is raised I'll proceed with this fix.
While you're at it, you should also fix your other versions of assign(). For instance, consider this flavor of self-assignment: optional<string> x("foo"); x = *x; // Assign to x from dereferenced x. This calls the following version of assign: void assign ( argument_type val ) { destroy(); construct(val); } Thus the code destroys *x, then attempts to assign to *x by dereferencing a pointer to uninitialized memory, resulting in undefined behavior. This is should be rewritten as void assign(argument_type val) { if (is_initialized) { get_impl() = val; } else { construct(val); } } Joe Gottman
"Joe Gottman" <jgottman@carolina.rr.com> escribió en el mensaje news:cv0qn4$b8j$1@sea.gmane.org...
"Fernando Cacciola" <fernando_cacciola@hotmail.com> wrote in message news:cuvnj8$urr$1@sea.gmane.org...
[SNIP]
void assign(argument_type val) { if (is_initialized) { get_impl() = val; } else { construct(val); } }
Joe Gottman
Well, even if you were wrong about this form of aliasing (and you're not), is clear that I must use this version anyway because I better be consistent about using T's operator=() instead of T::T(T const&). So good point, again ;) Cheers Fernando Cacciola
participants (7)
-
Bruno Martínez Aguerre -
David Abrahams -
Fernando Cacciola -
Jody Hagins -
Joe Gottman -
Matt Hurd -
Peter Dimov