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

[Edwin Vane]

================
Comment at: cpp11-migrate/PassByValue/PassByValueActions.cpp:77
@@ +76,3 @@
+
+/// \brief Find all references to \p ParmVarDecls accross all of the
+/// constructors redeclarations.
----------------
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.

================
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());
----------------
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?

================
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
----------------
Need a comma after `const-reference`.

================
Comment at: docs/PassByValueTransform.rst:78-80
@@ +77,5 @@
+
+If the parameter is used more than once no transformation is performed since
+moved objects have an undefined state. It means the following code will be left
+untouched:
+
----------------
Right after this example would be a good place for the class template behaviour blurb.


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