Uploaded image for project: 'Mesos'
  1. Mesos
  2. MESOS-2629

Update style guide to disallow capture by reference of temporaries

Agile BoardAttach filesAttach ScreenshotVotersWatch issueWatchersLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

Details

    • Task
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • None
    • 0.23.0
    • None

    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

      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)
      

      Attachments

        Activity

          This comment will be Viewable by All Users Viewable by All Users
          Cancel

          People

            jvanremoortere Joris Van Remoortere
            jvanremoortere Joris Van Remoortere
            Benjamin Hindman Benjamin Hindman
            Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Agile

                Completed Sprints:
                Mesosphere Q2 Sprint 8 - 5/1 ended 01/May/15
                Mesosphere Q1 Sprint 9 - 5/15 ended 15/May/15
                Mesosphere Sprint 10 ended 27/May/15
                Mesosphere Sprint 11 ended 08/Jun/15
                View on Board

                Slack

                  Issue deployment