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

[Edwin Vane]

================
Comment at: cpp11-migrate/PassByValue/PassByValue.h:25
@@ +24,3 @@
+/// \brief Subclass of Transform that uses pass-by-value semantic where
+/// applicable.
+///
----------------
I'd be a bit more specific in this one-line summary and replace `applicable` with `when move constructors are available to avoid copies`.

================
Comment at: cpp11-migrate/PassByValue/PassByValue.h:27-28
@@ +26,4 @@
+///
+/// Passing parameters by value is interesting when the object is copied. It can
+/// avoids a temporary by moving the resource of the parameter.
+///
----------------
I'm not sure I understand what this paragraph is saying. Perhaps be more concrete:

> When a class constructor accepts an object by const reference with the intention of copying the object the copy can be avoided in certain
> situations if the object has a move constructor. First, the constructor is changed to accept the object by value instead. Then this argument
> is moved instead of copied into class-local storage. If an l-value is provided to the constructor, there is no difference in the number of
> copies made. However, if an r-value is passed, the copy is avoided completely.

================
Comment at: cpp11-migrate/PassByValue/PassByValueActions.cpp:55
@@ +54,3 @@
+      if (To == ParamDecl) {
+        Count++;
+        if (Count > 1)
----------------
Should prefer pre-increment over post-increment if you don't need the original value.

================
Comment at: cpp11-migrate/PassByValue/PassByValueActions.cpp:77
@@ +76,3 @@
+
+/// \brief Find all references to \p ParmVarDecls accross all of the
+/// constructors redeclarations.
----------------
Typo `ParmVarDecls` -> `ParamDecl`.

================
Comment at: cpp11-migrate/PassByValue/PassByValueActions.cpp:78
@@ +77,3 @@
+/// \brief Find all references to \p ParmVarDecls accross all of the
+/// constructors redeclarations.
+static void
----------------
`constructors redeclarations` -> `redeclarations of \p Ctor`.

================
Comment at: cpp11-migrate/PassByValue/PassByValueActions.cpp:141-145
@@ +140,7 @@
+
+  // If the risk level is too strict do not apply the changes.
+  if (!Owner.isAcceptableRiskLevel(RL_Reasonable)) {
+    RejectedChanges++;
+    return;
+  }
+
----------------
Shouldn't this test be one of the first things done?

================
Comment at: docs/PassByValueTransform.rst:10
@@ +9,3 @@
+With move semantics added to the language and the standard library being updated
+accordingly (move constructors added to many types), it is now interesting to
+take an argument directly by value, instead of taking a const-reference and then
----------------
`being updated` -> `updated`.

I'd remove the parenthetical statement by replacing `accordingly (...),` with `with move constructors added for many types`. Note the removal of the comma.

================
Comment at: docs/PassByValueTransform.rst:11
@@ +10,3 @@
+accordingly (move constructors added to many types), it is now interesting to
+take an argument directly by value, instead of taking a const-reference and then
+copy. That way the compiler takes care of choosing the best way to construct the
----------------
`instead of taking a` -> `instead of by`. Remove the comma too.

================
Comment at: docs/PassByValueTransform.rst:12
@@ +11,3 @@
+take an argument directly by value, instead of taking a const-reference and then
+copy. That way the compiler takes care of choosing the best way to construct the
+copy.
----------------
`That way the compiler takes` -> `This transformation allows the compiler to take`

================
Comment at: docs/PassByValueTransform.rst:15
@@ +14,3 @@
+
+This is usually beneficial when the calling code pass an *rvalue* (assuming the
+move construction is a cheap operation). This short example illustrates how the
----------------
`This` -> `The transformation`
`pass` -> `passes`

Remove parentheses and `assuming` -> `and assumes`.

================
Comment at: docs/PassByValueTransform.rst:32
@@ +31,3 @@
+   Currently only constructors are transformed to make use of pass-by-value.
+   Contributions are welcome to add more transformations!
+
----------------
I'd reword to `Contributions that handle other situations are welcome!`.

================
Comment at: docs/PassByValueTransform.rst:42
@@ +41,3 @@
+Since `std::move()` is a library function declared in `<utility>` it may be
+necessary to add this include. The transform will make the insertion when
+necessary.
----------------
`make the insertion` -> `add the include directive`.

================
Comment at: docs/PassByValueTransform.rst:103-105
@@ +102,5 @@
+
+The situation where the generated code can be wrong is when the object
+referenced is modified before the assignment in the init-list through a "hidden"
+reference. An unlikely design, to say the least.
+
----------------
`The situation` -> `A situation`. Can't be guaranteed this is the only situation:)

Remove the last sentence.

================
Comment at: test/cpp11-migrate/PassByValue/basic.h:15
@@ +14,3 @@
+};
+
+struct UnmodifiableClass {
----------------
How about the case of a struct with a user-defined move constructor?

================
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());
----------------
I know this makes sense but it's not mentioned anywhere in code comments or in the documentation. You should probably update both.

================
Comment at: test/cpp11-migrate/PassByValue/basic.cpp:91
@@ +90,3 @@
+// Try messing up with macros
+#define MOVABLE_PARAM(Name) const Movable & Name
+struct I {
----------------
Have a CHECK: here that ensures the macro body wasn't changed. By the way, what part of the transform prevents this from happening?

================
Comment at: test/cpp11-migrate/PassByValue/basic.h:16
@@ +15,3 @@
+
+struct UnmodifiableClass {
+  UnmodifiableClass(const Movable &M);
----------------
Can you say why this isn't modifiable?


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

BRANCH
  pass-by-value

ARCANIST PROJECT
  clang-tools-extra