Self-exchange leaves us in a valid but unspecified state
7 points by aatango
7 points by aatango
This article seems misleading. Clearly std::exchange(x,x) is broken and should not be used. You didn’t actually explain what the problem is: in its first line when it moves obj to a temporary, it doesn’t expect that new_value is aliased to obj and is therefore destroyed (left in a moved-from state.) This is either a bug in the implementation, or the spec should spell out that this aliasing is not allowed.
Similar bugs can happen in implementations of operator= for move assignment; one should be careful to handle the special case where an instance is assigned to itself.
Trying to call this a way to clear x is … just silly, really.
Thank you for taking the time to read and comment. It's the first time that I try to write anything, so I appreciate it a lot!
First, you are correct that I failed to explain what is actually happening. I did not want to go too deep into move semantics and I could not think of a concise enough explanation for the behaviour. That is why I wrote only that we cannot really set expectations on what obj will be after the "self-exchange".
From my understanding, std::move() does not actually destroy anything. It simply casts it into an r-value. To be destroyed, that cast value must be used in a move constructor, or assignment, and that operation must itself destroy the original object, otherwise, it is left untouched. What am I missing here? I have heard enough times how complicated move semantics in C++ are that try to thread carefully around the topic.
That all of the main implementations appear to behave identically makes me wonder if it is a desired behaviour, albeit undefined in the standard.
And yes, I do completely agree that it is silly! I noticed the effect on std::string by chance, and I found this "hack" quite humorous. I am afraid my tone did not come across appropriately.
PS: I will update the post at the end of the week to, at least, make it clear that I do not endorse using this. I will wait a bit to see if I can update other points as well, rather than updating the post multiple times.
Your understanding is correct. The state of a moved-from value is unspecified, and normally you never see one, but it’s exposed here due to the aliasing error.
Prior to ARC, this kind of thing was a common bug in Objective-C:
[a release];
a = [b retain];
A lot of people wrote assignments like that, and it worked fine as long as a and b weren't the same object. But occasionally they would be. And that was sometimes fine, because if something else held a reference to the object (including an autorelease pool) then the first release wouldn't destroy it and the retain would work.
ARC explicitly made the assignment do the retain before the release (which was good style anyway, but required a temporary so people often didn't bother because it was more code. GNUstep had an ASSIGN macro that did the right thing), which eliminated a load of memory-management errors.
I would have expected std::exchange(a,a) to be a no-op. If the addresses of the two arguments are the same, then this should not do anything. I'd expect this because the C++ type system makes it really hard to statically prove that two references are not to the same object (this is one of the big selling points of Rust).
I don't understand why it is called broken. The spec that is even pasted at the end:
template<class T, class U = T> T exchange(T& obj, U&& new_value);
// Equivalent to:
T old_value = std::move(obj);
obj = std::forward<U>(new_value);
return old_value;
is very clear that the result will be: turn the object into whatever the type defines its moved-from state to be, followed by a self-assignment, which is maybe unspecified for standard library types, but is perfectly well defined for your own types, as you're the one writing what happens on move and operator=. If it's a std::vector it'll likely be an empty vector, if it's a boost::small_vector it'll likely won't change if its size was within the preallocated size, etc.