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

[Guillaume Papin]

  Thanks for the review. While testing the universal reference stuff an assertion was triggered in the handling of non const-ref parameters. I fixed the code and added two tests cases


================
Comment at: cpp11-migrate/PassByValue/PassByValueActions.cpp:77
@@ +76,3 @@
+
+/// \brief Find all references to \p ParmVarDecls accross all of the
+/// constructors redeclarations.
----------------
Edwin Vane wrote:
> Guillaume Papin wrote:
> > Edwin Vane wrote:
> > > Typo `ParmVarDecls` -> `ParamDecl`.
> > Fixed.
> > Now I'm wondering if there is a simple way to enable `-Wdocumentation` when building the migrator with CMake, I think it can catch these kind of issues.
> Might be able to use cmake's -DCMAKE_CPP_FLAGS_* options. I just have Doxygen installed locally and I run it looking for errors/warnings. In fact, you should probably do this. Internally we have a continuous integration project that builds docs nightly and it'll inform me if anything has been broken by commits that day.
I regularly check by using `make doxygen` in the docs/ directory but doxygen doesn't warn me for this kind of error (even if it's written "WARN_IF_DOC_ERROR = YES" in the Doxyfile). I get an error when `\param` is wrong though.

================
Comment at: docs/PassByValueTransform.rst:11
@@ +10,3 @@
+move constructors added for many types it is now interesting to take an argument
+directly by value, instead of by const-reference and then copy. This
+transformation allows the compiler to take care of choosing the best way to
----------------
Edwin Vane wrote:
> Need a comma after `const-reference`.
Fixed.

================
Comment at: test/cpp11-migrate/PassByValue/basic.cpp:100-106
@@ +99,8 @@
+// Test that templates aren't modified
+template <typename T> struct J {
+  J(const T &M) : M(M) {}
+  // CHECK: J(const T &M) : M(M) {}
+  T M;
+};
+J<Movable> j1(Movable());
+J<NotMovable> j2(NotMovable());
----------------
Edwin Vane wrote:
> Guillaume Papin wrote:
> > Edwin Vane wrote:
> > > I know this makes sense but it's not mentioned anywhere in code comments or in the documentation. You should probably update both.
> > Documentation added to on the matcher to explain that this kind of templates aren't matched.
> > 
> > I don't know what to write in the user documentation about this. Explaining what is changed makes sense but explaining every reason why we don't change something doesn't seem right to me. I think the transforms applied by the migrator should be something the people will be able to write themselves (an by extension understand).
> > 
> > And now that I'm thinking about this exact example I believe it will benefit from using an universal reference here.
> > 
> >   template <typename T> struct J {
> >     J(T &&M) : M(M) {}
> >     T M;
> >   };
> > 
> I agree generally that we don't have to talk about all the cases we don't change things but templates are special. It's clear why the test case shouldn't transform: the constructor arg type is a template parameter. But what about this:
> 
>   template <typename T>
>   struct K {
>     K(const Movable &M) : M(M) {}
> 
>     Movable M;
>     T A;
>   };
> 
> This is clearly changeable but I'm pretty sure it won't be unless you instantiate it. Then again, based on what you said in the matcher docs, perhaps not. Can you try it? Add a test case for it?
> 
> btw, why would the test case benefit from a universal ref?
Well, I'm a bit surprised but it works out of the box too! I added the test case.
So finally should I comment about the "template blurb behavior" in the documentation ? It seems to just do the Right Thing (TM) ?

Using an universal reference in my example will allow to keep the copy construction for const-reference parameters and for rvalue it will use the move construction directly, meaning that constructing a new object to move it is not even needed, we can just forward the argument as-is to the class field constructor.

  template <typename T> struct J {
    J(T &&M) : M(std::forward(M)) {}
    T M;
  };

After some reading this exact example is a bit dangerous. When a class has a unique constructor taking a single argument things get a bit more complicated (due to compiler generated constructors, such as the copy constructor). The following article explains some interesting things about this situation: http://ericniebler.com/2013/08/07/universal-references-and-the-copy-constructo/


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