jmlvanre/const_ref_to_temporary.md

## const_ref_to_temporary.md

      
    Raw
  

              const_ref_to_temporary.md
            
          
    Proposal:
We modify the style guide to disallow constant references to temporaries as a whole. This means disallowing both Case 1 and Case 2 below.
Background:

Case 1: Constant references to simple expression temporaries do extend the lifetime of the temporary till end of function scope:

a) Temporary returned by function:

// See full example below.
T f(const char* s) { return T(s); }

{
  const T& good = f("Ok");
  // use of good is ok.
}

b) Temporary constructed as simple expression:

// See full example below.
{
  const T& good = T("Ok");
  // use of good is ok.
}

Case 2: Constant references to expressions that result in a reference to a temporary do not extend the lifetime of the temporary:

a) Temporary returned by function:

// See full example below.
T f(const char* s) { return T(s); }

{
  const T& bad = f("Bad!").Member();
  // use of bad is invalid.
}

b) Temporary constructed as simple expression:

// See full example below.
{
  const T& bad = T("Bad!").Member();
  // use of bad is invalid.
}

Mesos Case:

In Mesos we use Future a lot. Many of our functions return Futures by value:

class Socket {
  Future<Socket> accept();
  Future<size_t> recv(char* data, size_t size);
  ...
}

Sometimes we capture these Futures:

{
  const Future<Socket>& accepted = socket.accept(); // Valid c++, propose we disallow.
}

Sometimes we chain these Futures:

{
  socket.accept().then(lambda::bind(_accepted)); // Temporary will be valid during 'then' expression evaluation.
}

Sometimes we do both:

{
  const Future<Socket>& accepted = socket.accept().then(lambda::bind(_accepted)); // Dangerous! 'accepted' lifetime will not be valid till end of scope. Disallow!
}


Reasoning:

Although Case 1 is ok, and considered a feature (http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/), Case 2 is extremely dangerous and leads to hard to track bugs.
If we explicitly allow Case 1, but disallow Case 2, then my worry is that someone coming along to maintain the code later on may accidentally turn Case 1 into Case 2, without recognizing the severity of this mistake. For example:

// Original code:
const T& val = T();
std::cout << val << std::endl;
// New code:
const T& val = T().removeWhiteSpace();
std::cout << val << std::endl; // val could be corrupted since the destructor has been invoked and T's memory freed.

If we disallow both cases: it will be easier to catch these mistakes early on in code reviews (and avoid these painful bugs), at the same cost of introducing a new style guide rule.

Performance Implications:

BenH suggests c++ developers are commonly taught to capture by constant reference to hint to the compiler that the copy can be elided.
Modern compilers use a Data Flow Graph to make optimizations such as

In-place-construction: leveraged by RVO and NRVO to construct the object in place on the stack. Similar to "Placement new": http://en.wikipedia.org/wiki/Placement_syntax
RVO (Return Value Optimization): http://en.wikipedia.org/wiki/Return_value_optimization
NRVO (Named Return Value Optimization): https://msdn.microsoft.com/en-us/library/ms364057%28v=vs.80%29.aspx


Since modern compilers perform these optimizations, we no longer need to 'hint' to the compiler that the copies can be elided.

Action Items:

Discussion on dev-list with community.
Remove all constant references to temporaries in code base.
Modify style guide with examples of bad behavior, and reasoning for the rule.

Example program:
#include <stdio.h>

class T {
public:
  T(const char* str) : Str(str) {
    printf("+ T(%s)\n", Str);
  }
  ~T() {
    printf("- T(%s)\n", Str);
  }
  const T& Member() const
  {
    return *this;
  }
private:
  const char* Str;
};

T f(const char* s) { return T(s); }

int main() {
  const T& good = T("Ok");
  const T& good_f = f("Ok function");

  const T& bad = T("Bad!").Member();
  const T& bad_f = T("Bad function!").Member();

  printf("End of function scope...\n");
}
Output:
+ T(Ok)
+ T(Ok function)
+ T(Bad!)
- T(Bad!)
+ T(Bad function!)
- T(Bad function!)
End of function scope...
- T(Ok function)
- T(Ok)