Rewrote ES.56, closed #514

CppCoreGuidelines.md

@@ -9592,73 +9592,113 @@ There can be code in the `...` part that causes the `delete` never to happen.
 Flag naked `new`s and naked `delete`s.
 
 
-### <a name="Res-move"></a>ES.56: Avoid `std::move()` in application code
+### <a name="Res-move"></a>ES.56: Write `std::move()` only when you need to explicitly move an object to another scope
 
 ##### Reason
 
-`std::move` is a cast in disguise.
-It does not move; instead, it allows code using its result to leave a useless object behind.
+We move, rather than copy, to avoid duplication and for improved performance.
+
+A move typically leaves behind an empty object ([C.64](#Rc-move-semantic)), which can be surprising or even dangerous, so we try to avoid moving from lvalues (they might be accessed later).
+
+##### Notes
+
+Moving is done implicitly when the source is an rvalue (e.g., value in a `return` treatment or a function result), so don't pointlessly complicate code in those cases by writing `move` explicitly. Instead, write short functions that return values, and both the function's return and the caller's accepting of the return will be optimized naturally.
+
+In general, following the guidelines in this document (including not making variables' scopes needlessly large, writing short functions that return values, returning local variables) help eliminate most need for explicit `std::move`.
+
+Explicit `move` is needed to explicitly move an object to another scope, notably to pass it to a "sink" function and in the implementations of the move operations themselves (move constructor, move assignment operator) and swap operations. 
 
 ##### Example, bad
 
-    struct Buffer {
-        zstring buf = new char[max];
-        int next = 0;    // next element to be written
-        void put(char ch) { buf[next++] = ch; /* ... */ } // put ch into buffer
-        // ...
-    };
+    void sink(X&& r);   // sink takes ownership of r
     
-    void maul(Buffer&& b)    // consume b and leave b destructable
+    void user()
     {
-        mybuf = b.buf;  // "steal" characters from buffer
-        buf = nullptr;  // make sure b is destructable
+        X x;
+        sink(r);            // error: cannot bind an lvalue to a rvalue reference
+        sink(std::move(r));  // OK: sink takes the contents of r, r must now assumed to be empty
         // ...
+        use(r);             // probably a mistake
     }
 
-    void test()
-    {
-        S s;
-        maul(std::move(s));
-        s.put('x');     // crash!
+Usually, a `std::move()` is used as an argument to a `&&` parameter.
+And after you do that, assume the object has been moved from (see [C.64](#Rc-move-semantic)) and don't read its state again until you first set it to a new value.
+
+    void f() {
+        string s1 = "supercalifragilisticexpialidocious";
+ 
+        string s2 = s1;             // ok, takes a copy
+        assert(s1=="supercalifragilisticexpialidocious");  // ok
+        
+        string s3 = move(s1);       // bad, if you want to keep using s1's value
+        assert(s1=="supercalifragilisticexpialidocious");  // bad, assert will likely fail, s1 likely changed
+    }
+
+##### Example
+    
+    void sink( unique_ptr<widget> p );  // pass ownership of p to sink()
+ 
+    void f() {
+        auto w = make_unique<widget>();
         // ...
+        sink( std::move(w) );               // ok, give to sink() 
+        // ...
+        sink(w);    // Error: unique_ptr is carefully designed so that you cannot copy it 
     }
  
-###### Alternative
+##### Notes
  
-Rvalue references are valuable for handling rvalues.
-If you define a function to take an rvalue reference to be able to simply and cheaply handle temporaries,
-also define an overload that takes lvalues.
+`std::move()` is a cast to `&&` in disguise; it doesn't itself move anything, but marks a named object as a candidate that can be moved from.
+The language already knows the common cases where objects can be moved from, especially when returning values from functions, so don't complicate code with redundant `std::move()'s.
 
-    void print(string&& s);         // print and consume (temporary) s
-    void print(const string& s);    // print and preserve the value of s
+Never write `std::move()` just because you've heard "it's more efficient."
+In general, don't believe claims of "efficiency" without data (???).
+In general, don't complicate your code without reason (??)
 
-An rvalue can be assumed not to be accessed after being passed.
-An lvalue must in general be assumed to be used again after being passed, that is after a `std::move`,
-so "careful programming" is essential to avoid disasters -- better not rely on that.
+##### Example, bad
 
-###### Note
-
-Standard library functions leave moved-from objects in a state that allows destruction and assignment.
-
-    void test()
-    {
-        string foo = "xckd";
-        string bar = std::move(foo);
-        foo = "kk";
-        cout << foo << "--" << bar << '\n';
+    vector<int> make_vector() {
+        vector<int> result;
+        // ... load result with data
+        return std::move(result);       // bad; just write "return result;"
     }
     
-This is valid code and prints `kk--xckd`, but for a general type even assignment isn't guaranteed to work.
-Whenever possible, follow the standard-library rule and make operations on rvalue leave the source object in an assignable and destructable state.
+Never write `return move(local_variable);`, because the language already knows the variable is a move candidate.
+Writing `move` in this code won't help, and can actually be detrimental because on some compilers it interferes with RVO (the return value optimization) by creating an additional reference alias to the local variable.
  
-##### Note
+##### Example, bad
  
-`std::move` (or equivalent casts) is essential for implementing move semantics and certain rare optimizations.
+    vector<int> v = std::move(make_vector());   // bad; the std::move is entirely redundant
+    
+Never write `move` on a returned value such as `x = move(f());` where `f` returns by value.
+The language already knows that a returned value is a temporary object that can be moved from.
+
+##### Example
+
+    void mover(X&& x) {
+        call_something( std::move(x) );         // ok
+        call_something( std::forward<X>(x) );   // bad, don't std::forward an rvalue reference
+        call_something( x );                    // suspicious, why not std::move?
+    }
+
+    template<class T>
+    void forwarder(T&& t) {
+        call_something( std::move(t) );         // bad, don't std::move a forwarding reference
+        call_something( std::forward<T>(t) );   // ok
+        call_something( t );                    // suspicious, why not std::forward?
+    }
 
 ##### Enforcement
  
-* Flag functions taking `S&&` arguments unless they have a `T&` overload to take care of lvalues.
-* Flag `std::move`s that are not function arguments passed as rvalue references
+* Flag use of `std::move(x)` where `x` is an rvalue or the language will already treat it as an rvalue, including `return std::move(local_variable);` and `std::move(f())` on a function that returns by value.
+* Flag functions taking an `S&&` parameter if there is no `const S&` overload to take care of lvalues.
+* Flag a `std::move`s argument passed to a parameter, except when the parameter type is one of the following: an `X&&` rvalue reference; a `T&&` forwarding reference where `T` is a template parameter type; or by value and the type is move-only. 
+* Flag when `std::move` is applied to a forwarding reference (`T&&` where `T` is a template parameter type). Use `std::forward` instead.
+* Flag when `std::move` is applied to other than an rvalue reference. (More general case of the previous rule to cover the non-forwarding cases.)
+* Flag when `std::forward` is applied to an rvalue reference (`X&&` where `X` is a concrete type). Use `std::move` instead.
+* Flag when `std::forward` is applied to other than a forwarding reference. (More general case of the previous rule to cover the non-moving cases.)
+* Flag when an object is potentially moved from and the next operation is a `const` operation; there should first be an intervening non-`const` operation, ideally assignment, to first reset the object's value.
+
 
 ### <a name="Res-del"></a>ES.61: delete arrays using `delete[]` and non-arrays using `delete`