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

[David Blaikie]

On Sun, Aug 11, 2013 at 9:17 AM, Guillaume Papin
<guillaume.papin at epitech.eu> wrote:
>
>
>   > 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.

Not sure if this has been discussed before, but tracking bugs in
llvm.org/bugs is helpful for project visibility. But do with that as
you will.

> I stick to this use case for now because I think it's already useful and the others introduce a lot more difficulty IMHO.

Agreed - just checking it's been thought about, etc.

>   > & 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?

I think it's still an acceptable thing - but I seem to recall (perhaps
incorrectly) that the migration tool had a concept of relative risk of
each transformation so a user could choose their "severity" of
transformation? My point was just to ensure this was appropriately
classified, not to suggest that we shuoldn't do it.

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

Fair point about exceptions - it wasn't what I had in mind. I probably
wouldn't try to suppress this transformation under that case because
it generally shouldn't matter (whether the copy has happened or not
shouldn't hurt (yes, it's possible to construct examples where it does
- I just mean such code isn't worth catering to, in my opinion) the
exception handling code)

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

Possibly a simpler example:

std::string s;

struct base {
  base() {
    s = "foo";
  }
};

struct derived: base {
  std::string mem;
  derived(const std::string &s) : mem(s) {
  }
};

int main() {
  derived d(s);
}

This is probably still rather contrived, I suppose... it's not
terribly realistic that someone would have intentionally designed this
behavior (a base class relying on such spooky action at a distance).
That's not to say that no one's ever done this, just that I'm not sure
it's helpful for a tool to worry about it.

>   > 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).

Yep. All the more reason to make types movable & as cheaply movable as possible.

>   > (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.

Agreed - thoughts for the future, do not need to be (& should not be -
for simplicity of reviewing) done in the first patch. Again, just
checking we've got the same direction in mind.

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