[MESOS-2629] Update style guide to disallow capture by reference of temporaries

• Type: Task
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 0.23.0
• Labels: tech-debt
• Target Version/s: 0.23.0
• Epic Link: Disallow Capture by reference of temporaries
• Sprint: Mesosphere Q2 Sprint 8 - 5/1, Mesosphere Q1 Sprint 9 - 5/15, Mesosphere Sprint 10, Mesosphere Sprint 11
• Story Points: 1
• Assignee: Joris Van Remoortere
• Reporter: Joris Van Remoortere
• Shepherd: Benjamin Hindman
• Created: 16/Apr/15 21:46
• Updated: 24/May/17 17:03
• Resolved: 02/Jun/15 10:28

Description

We modify the style guide to disallow constant references to temporaries as a whole. This means disallowing both (1) and (2) below.

Background

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

• 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.
    }
    

• Temporary constructed as simple expression:

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

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

• 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.
    }
    

• 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<T> 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 (1) is ok, and considered a feature, (2) is extremely dangerous and leads to hard to track bugs.
• If we explicitly allow (1), but disallow (2), then my worry is that someone coming along to maintain the code later on may accidentally turn (1) into (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.

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)