Hacker News Reader

Comment by olliej | Hacker News Reader

My reading of their moving assignment operator is that it is wrong, or at the very least unclear.

Basically they do std::swap on query field. In that case you are relying on the destructor of the moved object performing the delete (if necessary). I would argue that that falls into the “too clever” spectrum, and also kind of assumes that no one is going to refactor the destructor at some point in such a way that some other property will result in them incorrectly thing that the query is null, and so leaking it.

Having now read through this, I agree with others assessment - these questions are questionable at best. What is it they’re proven to do?

View on HN

I think relying on the destructor of the moved-from object running feels idiomatic to me. That's what the destructor is there for after all.

The destructor deleting its /own/ data. But here we are moving a bunch of the fields (and so clearing them) but swapping one of them. That seems like bad coding, and doesn’t match any idiom I’ve seen :-/

I wasn't too sure on some of the easy questions, but this swap... Oh god, my "someday, this is going to shoot someone in the foot"-sense started to tingle.

Move assignment has nothing to do with 'clearing fields' or whatever it's that the moved-to value steals what it can and leaves the moved from value in a valid but unspecified state. Notably, you should still be able to assign-to and destroy a moved-from value.

It's an invariant of the 'DirectorySearchResult' class that query is non-null, as the destructor deletes it without checking it for nullity. The most straight-forward and efficient way to achieve both the stealing and the valid state is to swap the pointers.

> It's an invariant of the 'DirectorySearchResult' class that query is non-null, as the destructor deletes it without checking it for nullity.

Deleting a null pointer is a fully specified no-op in C++.

That's true. And I see the other example answers they have written do assume the pointer can be null, although the original class makes no indication of this.

To make the question clearer they should have added a function on the original class that checks (or doesn't check) the pointer before dereferencing it, thereby clarifying whether query being null is valid or not.

for vectors it makes std::move a very fast operation

Vector has a good move assignment which also works. Why swap when you can move?

Swap is additionally risky because someone can ADL specialize it for their own purposes. (which is the point) And suddenly your move assignment is broken.

So people work around it by adding using std::swap clause there...

And then you're calling all those destructors.

std::move on vector zeroes out the source element array.

e.g. (pseudo-ish code)

operator=(vector&& foo) {

delete [] elements;

elements = foo.elements;

foo.elements = nullptr;

}

what the example code they have as an answer does is something morally equivalent to this:

operator=(vector&& foo) {

swap(elements, foo.elements);'

}

relying on the knowledge that because it's being moved the ~vector is going to be called immediately afterwards.

This strikes me as being "clever" in the "potentially confusing code, for no real reason" sense

actually that move assignment operator is not ideal. The foo parameter won't be destroyed immediately but only at the end of whatever scope (if any) it was at the call site.

The idiomatic copy and swap is like this:

T& operator =(T x) // note pass by value { std::swap(this, x); return this; }

This acts both as a normal and move assignment operator (if T is not copy constructible);

edit: how do you get HN to show asterisks? There should be one before each 'this'

Prefix each line with 4 spaces:

    T& operator =(T x) // note pass by value
    { std::swap(*this, x); return *this; }

Reply on news.ycombinator.com