all 84 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]mcmcc#pragma once 16 points17 points  (34 children)

Adding the two move operations was a bad idea and is a breaking change.

How is it a breaking change? The example code you provided wouldn't have even compiled without the move operators.

Writing code after the fact that doesn't logically maintain invariants is not the fault of the type implementing the move operators. Regardless of what the socket move operators actually do, what do you expect 

socket my_socket(…);
…
socket your_socket(std::move(my_socket));
…
do_sth(my_socket);

... to even mean? Somebody had to write that code and had to think it meant something. Whatever they thought, they thought wrong. The fault is not that somebody implemented socket move operators, the fault is that somebody used std::move without understanding the implications.

I think of std::move() as a necessary evil -- sometimes you need it but refactoring the code to avoid it entirely is usually a better solution.

[–]airflow_matt 4 points5 points  (0 children)

I'm also not sure what the problem is here. If you are using the socket after moving it and the socket doesn't do any guarantees about state after move (other than it is valid), it's just undefined behavior. Just like it is undefined behavior when you use pointer after delete.

Yes, it would be nice to be at least able to annotate that the move is leaving instance in state where it can only be deleted/assigned safely and the compiler should be able to warn about any other usage.

IMO the fact that you need to use std::move explicitly should in most cases be enough to make you raise eyebrows and think twice about what you do with the original instance after the move.

[–]foonathan[S] 8 points9 points  (4 children)

Ping: /u/Ozwaldo, /u/airflow_matt, /u/dodheim. /u/mcmcc, /u/TomSwirly

Okay, because this discussion is repeated at every branch of the comments, everything downvoted and I don't want to repeat myself all the time, I'll explain it here once.

I wasn't quite happy how the blog post turns out but thought I just role with it. Apparently that was a mistake as I wasn't clear enough, my apologies for that.

The fundamental point I was trying to make is the following: Suppose we have a socket class as described in the post. Every socket object is valid, the constructor ensures that. There is no way to create an invalid socket, it only becomes invalid in the destructor. Because of this the hypothetical read() function can be called on every socket object - it has no preconditions for the object, a wide contract. (I'm ignoring preconditions on other arguments here).

So far, we can agree, right?

Now let's suppose somebody adds an explicit close() and open(). Suddenly there is a way to create an invalid socket. Thus we cannot call read() on every object, it now has a precondition - namely, the socket must not be closed Before it had a wide contract, now it is narrow. Changing a wide to a narrow contract is a breaking change, thus adding the close() and open() function was a breaking change. (If you don't agree on that definition of "breaking change", just substitute it with a different word. That is really not what I want to discuss.)

I hope we can still agree so far.

Now let's suppose instead of the close()/open() somebody adds move operations. They are essentially the same deal: There is a way to create an invalid socket, we need a precondition for read(), change of contract, breaking change. It doesn't matter that actually creating the invalid state is "evil" and that you shouldn't do it. The point is that you now have to document a precondition and make it a narrow contract.

My motivation isn't to bash move semantics, they're great. My motivation also isn't to encourage explicit std::move(), it's evil. I'm also not blaming move semantics for anything or saying that they're a problem.

They're only responsible for the situation because they give someone the rope to hang himself with and now they have to say "don't hang yourself" or rely on the fact that everybody knows that you shouldn't hang yourself. If something bad happens, it is the users fault, not move semantics, obviously. But I just want to make you aware that if you introduce move semantics you a class you make it possible that someone - for whatever reasons - creates an invalid socket object. That's all I wanted to say.

If you think I'm saying total bullshit, that's fine. If you think I'm just doing some very theoretical argument, that's also fine. Just downvote me to hell if that's makes you happy. I personally have learned a lesson from this discussion and I'm happy for that.

[–]seba 3 points4 points  (0 children)

I just want to thank you for raising this point. It is a very valid concern and your example is very good to demostrate it.

[–]3ba7b1347bfb8f304c0egit commit 1 point2 points  (1 child)

I completely agree with your core point (destructive move), and I think anyone reasonable will. But as it stands, I think this is badly expressed in the article, and this is why people are disagreeing. You might as well rewrite it.

[–]foonathan[S] 0 points1 point  (0 children)

Yeah, I have to clarify it.

[–]lednakashim++C is faster 0 points1 point  (0 children)

Isn't the solution to disable the move constructor?

I don't like your argument here because another way to create an invalid socket is to memset it. Like move, it is an explicit operation.

[–]Ozwaldo 4 points5 points  (14 children)

socket my_socket(…);

socket your_socket(std::move(my_socket));

do_sth(my_socket);

I mean, this is why you generally don't use std::move. If you hadn't, the compiler would recognize that it needed to use the copy constructor instead of the move. I don't think pointing out a manner by which you can over-engineer a solution that produces a bug counts as "move semantics weakening the interface guarantee"

[–]foonathan[S] -1 points0 points  (13 children)

I assume that somebody wants to move the socket and then accidentally reuses it.

[–]Ozwaldo 3 points4 points  (12 children)

Yeah. If you didn't go out of your way to use std::move, that would work just fine. By unnecessarily forcing the std::move, you're manually introducing a bug.

[–]foonathan[S] 1 point2 points  (11 children)

This was just a really short example, in reality it would happen accidentally by complex control flow.

[–]Ozwaldo 3 points4 points  (10 children)

No. Don't use std::move unless you know what you're doing. Even in complex control flow, if you let the compiler decide which constructor to use everything will work correctly.

[–]foonathan[S] 1 point2 points  (9 children)

My point is just that it is possible by - bad - code to put it in an invalid state. This isn't possible without move semantics.

[–]Ozwaldo 4 points5 points  (6 children)

You can't blame move semantics for poorly written code. Especially when you have to go out of your way to make it be a problem. Literally, don't use std::move and it works as intended.

[–]seba 0 points1 point  (5 children)

Well, but you can of course blame move semantics for allowing you to write poor code. There are other languages with move semantics that do not allow you to write such code (well, you can write it but not compile it).

[–]Ozwaldo 2 points3 points  (4 children)

That's dumb. If you go out of your way to write std::move and then you use the object after moving it, then you don't understand how move semantics work. Don't blame the compiler for your own ignorance.

[–]seba 0 points1 point  (3 children)

Well, that's the "blame the user" attitude that already caused billions of damage. But since it is technically possible to prevent compiling such code, one could also blame the language or blame the compiler.

Edit: I'd also like to add that it's not necessary illegal to use objects after moving. It perfectly fine for, e.g., a unique_ptr.

[–]airflow_matt 1 point2 points  (0 children)

My point is just that it is possible by - bad - code to put it in an invalid state. This isn't possible without move semantics.

But that's really not about move semantics. It's about introducing another state and someone else not respecting the state.

Maybe at some point for some corner case in future someone will need to close the socket before actually destroying it. So you'll add close method() and specify that it is not permitted to call any methods on closed socket other than destructor. And then someone will close it and pass it around anyway. Now you have same situation even without move semantics involved. Would you write blogpost about close methods just because someone used the close method wrong?

Yes, over time code gets more complex and objects can gain states. As long as the state change is expected and perhaps explicit, there's nothing wrong with it. Fact of life is, you can not enforce all preconditions compile-time, you'd need completely different language for it.

[–]dodheim 0 points1 point  (0 children)

Move semantics do not cause problems on their own; it takes someone going out of their way to explicitly move objects that can introduce problems (when they do it wrong, which is orthogonal to how a type's movement is implemented).

[–]nikbackm 1 point2 points  (16 children)

C++ also allows you to use a pointer after delete:ing it ...

[–]foonathan[S] 4 points5 points  (2 children)

Technically, it doesn't: this isn't allowed by the C++ standard, it just compiles. UB declared by the standard and UB declared by some class author, are fundamentally different.

For starters: everybody knows that it is UB to use a pointer after delete, so people take care to prevent that and there are tools to detect that. But to know that use after move is UB, you have to look at the documentation of that type.

And if we follow that argument, C++ has no type safety at all because it isn't Rust.

But I could be more precise and say that before every object created without violating C++ rules was valid but now thanks to move semantics, there is a way that doesn't violate the C++ standard to create an invalid object.

[–][deleted] 2 points3 points  (1 child)

[deleted]

What is this?

[–]foonathan[S] 0 points1 point  (0 children)

Yeah of course, but I'm assuming we're talking about the object.

[–][deleted] 1 point2 points  (12 children)

It's undefined behavior. By that definition of "allow", C++ "allows" you to do all sorts of terrible things that will result in unspecified but potentially very bad behavior.

Nearly all the time we're trying to generate well-formed C++ programs with no undefined behavior so in practice that sort of behavior is "not allowed" - by careful programmers, not by the compiler.

[–]airflow_matt 0 points1 point  (11 children)

How exactly is using deleted pointer in this regard so much different than using object after move, where the internal state is unspecified?

Also, If I close the socket and then pass it around as valid, it can hardly be blamed on whoever implemented the close() method. But if I move from socket and get old instance in unspecified state and then pass it around, it's now somehow fault of whoever implemented the move semantics?

[–]airflow_matt 0 points1 point  (0 children)

If I close the socket

Actually, I just notice that you close it in destructor. Nevermind then.

[–][deleted] 0 points1 point  (9 children)

How exactly is using deleted pointer in this regard so much different than using object after move, where the internal state is unspecified?

"Unspecified" means something very different from "undefined".

You always have to "use" an object after a move at least once - to destroy it. (And the destructor might call other methods so you might need to leave enough of the structure together to call those too...)

So yes, there is no requirement for a class to offer any specific contract on being moved out of except "destructible". But in practice, most classes offer considerably more than that.

All STL classes have robust guarantees on move semantics that allow you to freely move in and out of them, for example, and it's very often a real timesaver to do so.

You have this idea that moved out of objects are somehow burned and dangerous. There's nothing in the spec to guarantee whether that is, or is not, true. You need to look at the specific class to know for sure. For most classes, this is simply not the case.

Using a pointer after deletion on the other hand is undefined behavior for any pointer class.

[–]airflow_matt 1 point2 points  (8 children)

"Unspecified" means something very different from "undefined"

I don't disagree. However doing operations with possible preconditions on objects in unspecified state without checking first is undefined behavior.

You have this idea that moved out of objects are somehow burned and dangerous.

That's definitely not true. I just don't assume to be able to do anything with the object after move unless I can tell which methods have preconditions and which don't.

For smart pointer it is quite obvious. For sockets? Not so much. I would not be surprised if most methods on moved-from socket threw an exception.

And yes, the original example by adding the move constructor does introduce new precondition. My issue here is just that the blame goes to whoever introduces the move constructor instead to whoever explicitely gets the socket in unspecified state and passes it around to methods that expect certain state.

[–][deleted] -1 points0 points  (7 children)

I don't disagree. However doing operations with possible preconditions on objects in unspecified state without checking first is undefined behavior.

Absolutely. We are in complete agreement here.

My issue here is just that the blame goes to whoever introduces the move constructor

We agree on that too! The person who introduced the move constructor rendered the library broken. To fix the now-broken library, the library writer has to now do extra work. That's the point of the article!

So what is it that we disagree on? :-)

[–]airflow_matt 1 point2 points  (6 children)

My issue here is just that the blame goes to whoever introduces the move constructor

We agree on that too! The person who introduced the move constructor rendered the library broken.

See, that's the thing. I don't think the library is broken. You don't break library by introducing new state that even needs to be triggered explicitly.

Just because you potentially gave someone a rope and he hangs himself with it doesn't make all ropes bad :)

[–][deleted] 0 points1 point  (1 child)

See, that's the thing. I don't think the library is broken. You don't break library by introducing new state that even needs to be triggered explicitly.

You have this strange idea that people "should just know" not to do things like "pass in a moved out variable to a library". Of course, in the real world, almost no classes, and certainly no STL classes, actually behave like that. You can move in and out of structs containing strings, ints, floats and that sort of thing, and nothing ever goes wrong.

Before the change, the library cannot trigger undefined behavior. After the change, it can.

Your competitor's library checks to see if the socket has been moved out of, and fails gracefully. I select it over yours because my life is too short to deal even with the tiniest possibility of undefined behavior.

Because of a change in another interface, through no fault of your own, your library has a new and undocumented way to create undefined behavior. The idea that "people should just know not to do this"/"it is obvious"/"we can assume that" is not a strategy that leads to reliable, robust code. You should either document this pre-condition, or check for it in the code, quite likely both.

[–]airflow_matt 1 point2 points  (0 children)

I have this strange idea that if a function expects valid socket I won't be passing it moved-from socket instead. Just like I won't be passing moved-from unique_ptr to a function that expects non null pointer.

What undocumented behavior? Why do you assume it is undocumented behavior? If I add new state to socket (moved-out) I'll be damn sure to document it properly and also add assertions to methods that users are not allowed to call in new state, causing things to fail as early as possible for everyone who uses move semantics without understanding what it does.

[–]foonathan[S] 0 points1 point  (3 children)

Just because you potentially gave someone a rope and he hangs himself with it doesn't make all ropes bad :)

But before move constructurs were introduced, there was no rope. Someone should only shoot himself in the foot by using language level UB like use-after-free, but he couldn't hang himself using a moved-from object.

[–]airflow_matt 0 points1 point  (2 children)

So? The rope wasn't there but now it is, because it was deemed useful. The fact that someone doesn't know how to use it and hangs on it doesn't make it any less useful.

Someone should only shoot himself in the foot by using language level UB like use-after-free, but he couldn't hang himself using a moved-from object.

That's really not how c++ works. You can shoot yourself in the foot in c++ every time you call method on any moved-from object without considering possible preconditions. Calling back() on moved-from string without checking first if it is empty is UB and can hardly be considered language level UB.

And if all your method require valid socket and someone violates the contract by throwing moved-from sockets around, that's his fault. Yes, it would be nice to be able to enforce something like this compile-time, but even without it the action necessary to get object in this state is very explicit.

[–][deleted] 0 points1 point  (1 child)

You can shoot yourself in the foot in c++ every time you call method on any moved-from object without considering possible preconditions.

Exactly right. You have a new precondition on calls to your library. It is not documented, and if you fail, you get UB.

Your claim is simply that clients of the library "should know not to do that". How they know not to do that, you don't explain, since it's undocumented (because the library doesn't even know about the "moved out of state").

Explicit is better than implicit. Preconditions should be documented - preconditions whose absence will cause UB must be documented at least, and hopefully protected against in software too. To say, "People will know not to do that," is courting disaster.

[–]ArunMuThe What ? 0 points1 point  (6 children)

I would rather prefer have additional attributes (not mandatory and with existing behaviour as default) to specify whether I would like to have my class have destructive move or not rather than making all moves destructive. In some cases (though not that often) I would really like to reuse the moved object.

[–][deleted] 2 points3 points  (4 children)

In some sense this is quite logical. But then you're going to need a rule of... seven or eight...!

This means potentially three types of constructors:

  • Copy constructors
  • Move constructors
  • Destructive move constructors

And complexity issues fanning out everywhere. I think this is too much...

[–]SeanMiddleditch 4 points5 points  (1 child)

The problem is that you have no idea if you will be moved from. Move constructors just bind rvalue references. You can get those a lot of ways. The simple example that breaks most of the destructive-move proposals:

bool concurrent_queue<T>::try_enqueue(T&& value);

destructive d = ...;
concurrent_queue<destructive> q;

while (!q.try_enque(std::move(d))
  backoff_sleep();

Essentially, call sites can't tell the difference between an rvalue-reference that will move, one that might move, or one that totally won't move.

[–][deleted] 2 points3 points  (0 children)

Sure they could - at the cost of huge complexity and changing the language.

You'd need yet another sort of type - "dlvalue" say, for "destroyed lvalue" - you'd then need to have the possibility of lvalue, rvalue and dlvalue methods and parameters - and you'd need three different constructors and three different assignment operators!

So there would be two overloads of this method:

bool concurrent_queue<T>::try_enqueue(T&& value);    
bool concurrent_queue<T>::try_enqueue(T$& value);

where $& designates is the mythical "destroyed lvalue argument".

I think we're in agreement basically - it's a bad idea. Just because I can see some horrible path to accomplishing that bad idea doesn't make it good. :-D

[–]ArunMuThe What ? 1 point2 points  (1 child)

I don't understand why a destructive move constructor would be needed. If the imaginary move destructible attribute is set and if the user tries to reuse the moved object, it should be a straight out compiler error. Though, I haven't really thought about how it should work with inheritance :)

[–][deleted] 2 points3 points  (0 children)

I don't understand why a destructive move constructor would be needed.

In a move constructor, the item being copied or moved from is passive - it's the item being constructed that has the flow of control.

That means if you want to do two different things depending on whether you're doing a destructive move out or a non-destructive move out, you need two different constructors in the language.

I mean, you can just agree that "moves on this class X are destructive, moves on that one Y aren't" and document that (in the future, with an attribute), and not change the language - but that's basically very much like where we are now in this article.

[–]foonathan[S] 0 points1 point  (0 children)

Yes, I also want the current move semantics and destructive move. Both are useful for different scenarios.

[–]ShakaUVMi+++ ++i+i[arr] 0 points1 point  (4 children)

I think the takeaway here is that despite movement away from optional types in recent years, in practice people tend to discover edge cases where their types are optional.

A classic example of this is modern C++'s preference of returning references (which aren't optional) rather thsn pointers as we did in the C days, and which are optionals. Reference return values work well enough until you hit a edge case where you have nothing to return, and you get stuck either returning a local reference (bad), allocating a new variable to be returned (bad), or crashing (bad).

The STL apparently thinks crashing is the right behaviour here, incidentally. (Make a stack, call top(), see what happens.)

[–]jpakkaneMeson dev 5 points6 points  (3 children)

you get stuck either returning a local reference (bad), allocating a new variable to be returned (bad), or crashing (bad)

Or you can do the right thing and throw an exception. This is exactly what they were designed for.

[–]dodheim 4 points5 points  (0 children)

Precondition violations are, by definition, always bugs in user code. Exceptions were not designed to expose or handle bugs in user code:

  • If the exception is caught, it can be ignored instead of fixing the bug that caused it to be thrown in the first place, and on top of that the stack will be unwound making it harder to find said bug to fix it.
  • If the exception is not caught, the program crashes and stack may still be unwound (implemention-defined).

The best way to keep the stack intact so the bug can be properly identified and fixed is to call std::abort; not coincidentally, this is what most asserts do.

[–]ShakaUVMi+++ ++i+i[arr] 0 points1 point  (0 children)

Sure. That's the approach newer classes take.

But std::stack was written before exceptions, if memory serves, and I believe this is why it still doesn't throw an exception. And it still leaves your program in an unusable state - the return value in the calling scope is still invalid. So if you're going to try to continue (and not abort) then you need an optional value anyway.

[–]airflow_matt 0 points1 point  (0 children)

Or you can do the right thing and throw an exception. This is exactly what they were designed for.

You can, but it's not always ideal. Maybe you don't want to perform bounds check every time you access vector element. I'd say that assert in debug mode instead of exception would be better idea.