Proposal idea: std::move should not accept a const-qualified argument

[Max Galkin]

/* Just floating the idea, all and any feedback appreciated! :) --- Max */

This is a proposal to tighten the definition of std::move so that it would 

not accept a const-qualified argument, and, therefore, never return a const

rvalue reference. Calling std::move with a const-qualified argument should 

result in a compilation error, explaining that such a value is not movable.

Presumably, the intent of calling std::move is always to obtain a mutable 

rvalue reference, never a const rvalue reference. Const rvalue references

in general only have corner case applications, e.g. by STL [1].

In all cases known to the author of the proposal, when std::move was called 

with a const-qualified argument, it indicated a programmer's error. Usually

such an error does not lead to incorrect code, only to less efficient code,

because instead of a move-aware overload a copying overload of the called 

function is used. But since move semantics is a performance-enabling feature,

it seems practical to prevent such ways to accidentally misuse std::move.

This will encourage correct and efficient use and reduce the number of 

"gotchas" in the standard library.

Note that this proposal is a breaking change, because it reduces the set of 

acceptable programs, but, if there are indeed no benefits from calling 

std::move with a const-qualified value, then all the broken cases will be

incorrectly and inefficiently applied std::move calls. Hence this change 

may uncover performance defects, but it will not break any "proper" calls,

which produce mutable rvalue references.

# std::move misuse cases this change will break/prevent.

1. std::move attempting to move from a const local variable.

2. std::move attempting to move from a const member field 

during move construction.

3. Accidental mistake in move constructor signature leading to

an std::move attempting to move from a const rvalue:

  class X {

  private:

    vector<int> m_v;

  public:

    X(const X&& other) : m_v(std::move(other.m_v)) { };

  };

# Potential new implemenation of std::move.

  template <typename T> constexpr remove_reference_t<T>&& 

  move(T&& t) noexcept

  {

    static_assert(!std::is_const<remove_reference_t<T>>::value,

      "std::move does not accept const-qualified arguments");

    return static_cast<remove_reference_t<T>&&>(t);

  }

# References

[1] CppCon 2014: Stephan Lavavej "STL Features And Implementation 

Techniques", minutes 41-47:

https://www.youtube.com/watch?v=dTeKf5Oek2c&feature=youtu.be&t=2501

[Howard Hinnant]

On May 9, 2015, at 12:26 AM, Max Galkin <maksim...@gmail.com> wrote:
>
> Note that this proposal is a breaking change, because it reduces the set of
> acceptable programs, but, if there are indeed no benefits from calling
> std::move with a const-qualified value, then all the broken cases will be
> incorrectly and inefficiently applied std::move calls. Hence this change
> may uncover performance defects, but it will not break any "proper" calls,
> which produce mutable rvalue references.

It is indeed useful to sometimes call std::move on const objects, and have those objects fall back to copying (even at a performance loss). Doing otherwise will effectively ban the use of const-qualified types in generic code.

Consider for example some wrapper type:

template <class T>
class my_wrapper
{
T t_;
public:
my_wrapper() = default;

my_wrapper(T t)
: t_(std::move(t))
{}

my_wrapper(my_wrapper&& x)
: t_(std::move(x.t_))
{
std::cout << "And do some other work\n";
}
};

template <class U, class ...T>
my_wrapper<U>
make_wrapper(T&& ...t)
{
return make_wrapper<U>(std::forward<T>(t)...);
}

For whatever reasons, the move constructor of my_wrapper can’t be implemented with “= default” because it needs to do some additional work. So this move constructor moves the wrapped object and does whatever other work is appropriate for this user-defined wrapper.

But clients often want to put const-qualified types in wrappers (I see them all the time in pair and tuple for example). But if we were to make this change, then clients will no longer be able to do this:

auto m1 = make_wrapper<const std::string>();

error: static_assert failed "std::move does not accept
const-qualified arguments"
static_assert(!std::is_const<remove_reference_t<_Tp>>::value,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test.cpp:16:19: note: in instantiation of function template specialization 'std::move<const std::string&>' requested here
: t_(std::move(x.t_))
^
test.cpp:32:15: note: in instantiation of member function 'my_wrapper<const std::string>::my_wrapper' requested here
auto m1 = make_wrapper<const std::string>();
^
1 error generated.

std::move has always been a *request* to move, not a demand to move. And my_wrapper is a prime example of why it is only a request. Making it more than a request will break large amounts of generic code. And fixing that code, while possible, will make the generic code far more complex. It would have to manually branch on is_const (at compile time) and copy instead, obfuscating otherwise simple code.

Howard

[Max Galkin]

Thank you for the response, Howard! This is an interesting example.

I assume you meant "return my_wrapper<U>" in place of "return make_wrapper<U>"

in the make_wrapper method.

I have a question about the example. Instead of the field "T t_;", maybe we should

be using "remove_const_t<T> t_;" in this wrapper? It would make the move

constructor work even for const string. What is the benefit of passing the const

qualifier to the type of the private field? If accessor methods need to be exposed,

then those would expose const reference, of course.

It is possible that I'm missing something?..

Also, we could add something like "std::rvalue_cast", which would perform the cast

unconditionally (like the current std::move does). I haven't mentioned this option in the 

original proposal email, because I hope that this cast, potentially producing const rvalue

references, is not needed in practice...

Max

[Arthur O'Dwyer]

On Saturday, May 9, 2015 at 12:34:27 PM UTC-7, Max Galkin wrote:

I have a question about the example. Instead of the field "T t_;", maybe we should

be using "remove_const_t<T> t_;" in this wrapper? It would make the move

constructor work even for const string. What is the benefit of passing the const

qualifier to the type of the private field?

Well, you're basically proposing that every writer of generic code complicate their code, just so that you can detect one particular class of uncommon typographical errors. There's a huge (and hopefully obvious) benefit to not force everyone to write remove_const_t<T> when they could just be writing T. And there is just a ton of code out there right now just writing T; I'm sure you couldn't force everyone to change it if you tried.

As Howard says:

> std::move has always been a *request* to move, not a demand to move.  And my_wrapper is a prime example of why it is only a request.  Making it more than a request will break large amounts of generic code.  And fixing that code, while possible, will make the generic code far more complex.  It would have to manually branch on is_const (at compile time) and copy instead, obfuscating otherwise simple code.

 

Also, we could add something like "std::rvalue_cast", which would perform the cast

unconditionally (like the current std::move does). I haven't mentioned this option in the 

original proposal email, because I hope that this cast, potentially producing const rvalue

references, is not needed in practice...

Current std::move already has the semantics that you propose for std::rvalue_cast, so let's keep using std::move for that.

The question is, do we need another identifier that means "like move but only for non-const-qualified types"?

I've found that a good practice when thinking about const is to remember that const is not the only type-qualifier in C++! Therefore, it basically never makes sense to propose anything re: const if you are not equally prepared to propose the same thing re: volatile.

http://en.cppreference.com/w/cpp/types/remove_cv

http://en.cppreference.com/w/cpp/types/is_const#See_also

–Arthur

[David Krauss]

On 2015–05–09, at 1:26 PM, Max Galkin <maksim...@gmail.com> wrote:

Const rvalue references

in general only have corner case applications, e.g. by STL [1].

In all cases known to the author of the proposal, when std::move was called 

with a const-qualified argument, it indicated a programmer's error. 

A const rvalue reference will usually initialize a const lvalue reference. This doesn’t indicate an error.

The source of the performance issue you’re trying to detect isn’t usually at the move(const_value) at all, but somewhere upstream where someone forgot to move and the object got bound to a const & reference instead. If, in the troublesome context, the move really wasn’t forgotten but actually unsafe (and you can’t assume it is), your proposal would take an innocuous corner case and make it intractable.

A generic function working with both const and non-const sequences can indiscriminately move and get correct, optimal behavior in both cases. (There are classes with movably mutable members, but they should work either way and the intent is ambiguous.) My proposal N4166, (to be revised soon) provides new std::initializer_list types with non-const (movable) access, and the generic usage style is just to ask for move and take what you get.

Anytime you pass an expression of const class type to a forwarding reference, it will generate a const rvalue reference. Do you propose to deprecate const prvalues in general?

Usually

such an error does not lead to incorrect code, only to less efficient code,

because instead of a move-aware overload a copying overload of the called 

function is used.

There are several non-breaking solutions:

1. Implement your proposal in your own codebase. Usage experience will help if you still feel it to be worthy of standardization.

2. static_assert the non-constness in specific instances of calls to move, or when receiving rvalue references by forwarding.

3. Delete the const move constructor of the offending classes: T::T(T const &&) = delete;.

[Max Galkin]

Thanks for taking time to review the idea, Arthur!

> Well, you're basically proposing that every writer of generic code complicate their code, just so that you can detect one particular class of uncommon typographical errors.

I'm not proposing this to detect problems in *my* code, 

personally I can just use this utility function directly :)

I'm proposing this because I think it closes a "gotcha" in the library,

where a copy is made instead of the move implicitly in some subtle cases

which I'm afraid are usually unnoticed.

Which is better for writers of generic code: a function that silently chooses

a less efficient approach depending on the constness of the argument, 

or a function that always produces a mutable rvalue reference, and reports an error

when it can't, so that the writer can take corrective action depending on

the purpose of the library -- either get rid of the constness, or change the

"move" to uncoditional cast?

It can be a new identifier, not "std::move", but shouldn't library writers

prefer to use the more restrictive identifier as their default option anyway

to catch the errors early on and at compile time?

> There's a huge (and hopefully obvious) benefit to not force everyone to write remove_const_t<T> when they could just be writing T. 

If they just write "T" in that wrapper example, and instantiate with "const string", 

the move constructor will _copy_ the private string, because the field will be of type "const string".

If they write "remove_const_t<T>", the move constructor will _move_ the private "string".

Difference in wrapper performance may be significant!

Please correct me, I must be wrong, because it is not obvious to me, what is the benefit of just writing T instead of remove_const_t<T>.

> As Howard says:

>> std::move has always been a *request* to move, not a demand to move.  And my_wrapper is a prime example of why it is only a request.  Making it more than a request will break large amounts of generic code.  And fixing that code, while possible, will make the generic code far more complex.  It would have to manually branch on is_const (at compile time) and copy instead, obfuscating otherwise simple code.

I undestand the current implementation is a *request*.

I don't understand why is *request* better than demand.

For "my_wrapper" see the above explanation. 

You don't need to branch on "is_const" in this case...

> I've found that a good practice when thinking about const is to remember that const is not the only type-qualifier in C++! Therefore, it basically never makes sense to propose anything re: const if you are not equally prepared to propose the same thing re: volatile.

One can move volatile-qualified objects, but can't move const-qualified objects, so there is already an asymmetry by design...

I have examined the 2 pages you linked and couldn't find any clarification there...

So far it seems to me that "volatile" isn't really interacting with move in any interesting way... Can you elaborate on how "volatile" could be relevant here?

Thanks!

Max

[Max Galkin]

Thank you for reviewing the idea, David!

> A const rvalue reference will usually initialize a const lvalue reference. This doesn’t indicate an error.

> The source of the performance issue you’re trying to detect isn’t usually at the move(const_value) at all, but somewhere upstream where someone forgot to move and the object got bound to a const & reference instead. If, in the troublesome context, the move really wasn’t forgotten but actually unsafe (and you can’t assume it is), your proposal would take an innocuous corner case and make it intractable.

In what context is this happening, the situation you're describing?

1. If you're in a non-generic method, and you're trying to move a variable declared as "const&" then what are you trying to achieve? It is just misleading and has no effect.

2. If you are in a generic method using a universal reference, then you should probably be calling std::forward instead of std:move.

3. If you're inside the move constructor intialization and some field is const, you can just initialize directly, without calling std::move() on it.

4. I can really only think of a case such as 

template<class T> 

void f1(T& t)

{ 

   f2(std::move(t)); 

}

called with a const object... Could it maybe be turned into taking a universal reference and forwarding...

What would be a real application for this?.. Or for an uncoditional move of a forwarding reference?..

I'll think about it :)

However, in any case, nothing becomes intractable with this proposal. An unconditional cast to T&& is always available... 

The question is, should the unconditional cast be the default, or should a more restricitve approach be preferred, and only

switch to an unconditional cast, if the restrictive move is not appropriate...

> A generic function working with both const and non-const sequences can indiscriminately move and get correct, optimal behavior in both cases. (There are classes with movably mutable members, but they should work either way and the intent is ambiguous.) My proposal N4166, (to be revised soon) provides new std::initializer_list types with non-const (movable) access, and the generic usage style is just to ask for move and take what you get.

Are you referring to this example below from N4166?

   template< typename t > // t is foo or foo &&.

   void generic(std::initializer_list< t > seq) {

      for (auto && f : seq) { // f is const or modifiable.

         smth(std::move(f)); // Move is defeated by const.

      }

   }

Well, this is interesting... :)

I understand that you are relying on the implementation detail that initializer_list 

sequence always has either const or effectively-rvalue elements and never just lvalues

since one can't make initializer_list<T&> and pass it to 2 different objects...

To convince myself that this method is safe I had to review the declarations of

both classes and hope that these implementation details will not change...

Maybe the iterator type could be a wrapper that would return a T&& on operator*

and then the usage would be to std::forward<decltype(f)>(f)? 

Would it make more sense?

Thanks!

Max

[Nicol Bolas]

On Sunday, May 10, 2015 at 3:06:59 AM UTC-4, Max Galkin wrote:

The question is, should the unconditional cast be the default, or should a more restricitve approach be preferred, and only

switch to an unconditional cast, if the restrictive move is not appropriate...

There's a more relevant question that really ought to be answered first: is the problem you cite significant enough to potentially break the world? There's been a lot of code written that uses std::move. Some of it in places where it could get a `const&`. You claim that these are "broken", because they can do a copy instead of a move. I claim that it is functional code even if it copies; we have lots of code that works this way right now.

Why should we break their "broken" code that works just fine?

Until you can answer that, I'd say that your question here is rather moot. std::move is what it is. Adding an alternative would be fine; changing it takes more than your belief that it's not good as currently specified.

[David Krauss]

On 2015–05–10, at 3:06 PM, Max Galkin <maksim...@gmail.com> wrote:

In what context is this happening, the situation you're describing?

I’m just inventing a situation where potential performance is lost, because you mentioned that such situations exist. Your examples didn’t involve loss of performance; they were all merely about failure to gain performance by applying move where it’s not very applicable.

1. If you're in a non-generic method, and you're trying to move a variable declared as "const&" then what are you trying to achieve? It is just misleading and has no effect.

I agree, it might be helpful to get a warning when std::move is applied to a local variable declared with the const keyword. But it’s only something for -Wextra.

2. If you are in a generic method using a universal reference, then you should probably be calling std::forward instead of std:move.

Unless the caller didn’t know the object was ready to expire. Generic methods can sometimes take the initiative to move.

3. If you're inside the move constructor intialization and some field is const, you can just initialize directly, without calling std::move() on it.

This isn’t an error, such a move() is harmless and it does what the implicit definition would do.

It’s an error to declare something as const because it shouldn’t change… only until it’s moved. Fields should be const only rarely.

Well, this is interesting... :)

I understand that you are relying on the implementation detail that initializer_list 

sequence always has either const or effectively-rvalue elements and never just lvalues

since one can't make initializer_list<T&> and pass it to 2 different objects…

initializer_list<T>::iterator always yields lvalues; typically it’s only T const*. But it only needs to be const as long as two list objects might be sharing the sequence, which is an aliasing problem. Objects simply don’t have value category; lvalue or rvalue is only part of the way you observe an object.

You are allowed to move from the sequence elements whenever they are modifiable, because the modifiable version of initializer_list is only move-constructible, like unique_ptr. If the caller passed the container by copy instead of by rvalue or move(), you wouldn’t get the chance to move from the sequence elements.

It would be a lot simpler if the corner case of copying an initializer_list simply didn’t exist. But, I have to design around the existing warts.

To convince myself that this method is safe I had to review the declarations of

both classes and hope that these implementation details will not change…

Safe from what?

The sequence is an array. This is fundamental to initializer_list. The details will not change because they’re standardized, for better or worse.

Maybe the iterator type could be a wrapper that would return a T&& on operator*

and then the usage would be to std::forward<decltype(f)>(f)? 

Would it make more sense?

No, that would be horrible. Never start a move without being obvious about it.

Asking for a move but not getting one: safe.

Merely mentioning some object and getting it erased: unsafe.

By the way, we do already have std::move_iterator, and it works only as long as the generic algorithm receiving it doesn’t need to access the same thing twice. Perhaps there’s a generally safe methodology for move iterators, but it hasn’t been discovered yet.

[Max Galkin]

Thanks, David, I think your initializer_list proposal has a true counterexample, where you need an unconditional cast to rvalue ref no matter whether the value is const or not. At least this seems to be the simplest approach.

Question to all: what if the proposal would be to just add a new utility method with a conditional cast only for non-const values? Without breaking the std::move.

I don't know what the name would be, but maybe "std::make_movable" for the sake of this argument.

Would this be a useful utility, or does it not make the bar for being a useful proposal?

Thanks!

  template <typename T> constexpr remove_reference_t<T>&& 

  make_movable(T&& t) noexcept

  {

    static_assert(!std::is_const<remove_reference_t<T>>::value,

      "Cannot make a const-qualified argument movable.");

    return std::move(t);

  }

----

Max

[David Krauss]

On 2015–05–12, at 2:23 PM, Max Galkin <maksim...@gmail.com> wrote:

  template <typename T> constexpr remove_reference_t<T>&& 

  make_movable(T&& t) noexcept

  {

    static_assert(!std::is_const<remove_reference_t<T>>::value,

      "Cannot make a const-qualified argument movable.");

    return std::move(t);

  }

If you want to do it yourself, there’s a better way:

namespace carefully {
    using std::move;
    
    template< typename t >
    t const && move( t const & ) = delete;
}

const int k = 3;

int a = carefully::move( 5 ), // rvalue
    b = carefully::move( a ), // non-const lvalue
    c = carefully::move( k ), // const lvalue => ERROR
    d = carefully::move( std::move( k ) ); // already a const rvalue

http://coliru.stacked-crooked.com/a/dca0d93116346b43

This only stops cases where rvalue qualification is actually being added, and in other cases, it genuinely uses std::move just in case it has special status within the implementation or to some static analyzer, and it doesn’t incur any additional template instantiations.