[PATCH] cpp11-migrate: Add a transform that use pass-by-value in constructors

[Guillaume Papin]

  > While constructors are an interesting case - do you intend to/is there any reason this hasn't been generalized over arbitrary functions that involve taking exactly one copy of a parameter & otherwise not referencing that parameter?

  I agree with you, I tried to make the structure of the code and the documentation open to new additions of the same kind. Assuming the transform gets accepted I will log an issue for enhancement in JIRA. I stick to this use case for now because I think it's already useful and the others introduce a lot more difficulty IMHO.

  > & it's clear that this is an unsafe transformation, right? If, in the original example, the parameter object is referenced indirectly elsewhere (eg: another reference parameter has an indirect reference to the movable parameter, or the movable parameter is a reference to a global) causing the copy to happen at the time the argument was passed, rather than when the original code copied it could cause an observably different object to be copied. I don't think that negates the value in having this transformation - just something to be aware of.

  You are right. I put it as a reasonable risk, do we agree on this or this should be considered risky? The two cases I see (but I think I may have a limited imagination here) are:
  1. The exception locality, if the constructor throws it will throw at a different place (outside the constructor). Should I look for a function-try block and avoid transforming if one is present? Unless we known the constructors can't throw.
  2. If the object referenced is modified before in the init-list by a hidden reference. The best example I could come up with is:

    #include <string>
    #include <iostream>

    struct StringHolder {
      std::string S;
    };

    struct EvilBase {
      EvilBase(StringHolder &H) { H.S = "Evil things just happened!"; }
    };

    struct A : EvilBase {
      A(StringHolder &H, const std::string &S) : EvilBase(H), S(S) {}

      void dump() { std::cout << S << "\n"; }

    private:
      std::string S;
    };

    int main() {
      StringHolder H{ "I won't get printed!" };
      A a(H, H.S);
      a.dump();  // prints "Evil things just happened!\n"
      return 0;
    }

  Did you happen to have something like this in your mind or a less contrived example? I would like to document why the transform is risky and if there is a better example than that I guess it's better.

  > Also the "pass by value" approach here isn't optimal but is the nicest/simplest way to write this - in the case where the caller passes a non-temporary there will be one copy construction + one move construction when pass by value is used. In the original code (pass by const ref) there will be one copy construction and no move construction. So the really efficient way is to have two functions - one for rvalue ref (that move constructs) and one for const ref (that copy constructs). But that results in a ridiculous explosion of functions that's just silly - making the pass by value quite sensible as a default until you find some reason to need to split the two functions (because the cost of the extra move is unacceptable in some particular case for some reason)

  I agree with you. There is an article about that explains this issue (http://www.codesynthesis.com/~boris/blog/2012/06/19/efficient-argument-passing-cxx11-part1/). It shows how difficult it is to make one function of the parameter with an rvalue and one without. If you get more than one parameter to transform it gets super-messy very fast (with N being the number of argument copied it requires 2^N overloads).

  > (of course, as an extension - detecting read-only operations on the movable object & replacing references to the parameter with references to the copy (if it hasn't been mutated) could be cool too, but much more work I assume)

  Yeah I thought about this one too but it gets messy if the reference is done before the assignment for example

    struct A {
      A(const std::string &M) : Ref(M), Copy(M) {}
      const std::string &Ref;
      std::string Copy;
    };


  Or if one of the reference is used to compare against the newly made copy.

    struct A {
      A(const std::string &S) : Copy(S) {

        // suppress duplicate spaces in Copy
        Copy.erase(std::unique(Copy.begin(), Copy.end(), [](char c1, char c2) {
          return c1 == ' ' && c2 == ' ';
        }),
                   Copy.end());

        if (Copy != S)
          <do something special if the original value has changed>;
      }
      std::string Copy;
    };


  And more things I'm sure. I think this is part of the enhancements but IMHO in its current state the transform is already useful with a very low possibility of error.

http://llvm-reviews.chandlerc.com/D1342