[PATCH] cpp11-migrate: Add a transform that use pass-by-value in constructors
Guillaume Papin
guillaume.papin at epitech.eu
Thu Aug 22 04:16:25 PDT 2013
================
Comment at: cpp11-migrate/PassByValue/PassByValue.h:25
@@ +24,3 @@
+/// \brief Subclass of Transform that uses pass-by-value semantic where
+/// applicable.
+///
----------------
Edwin Vane wrote:
> I'd be a bit more specific in this one-line summary and replace `applicable` with `when move constructors are available to avoid copies`.
Done.
================
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.
+///
----------------
Edwin Vane wrote:
> 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.
This is definitely better.
Done.
================
Comment at: cpp11-migrate/PassByValue/PassByValueActions.cpp:55
@@ +54,3 @@
+ if (To == ParamDecl) {
+ Count++;
+ if (Count > 1)
----------------
Edwin Vane wrote:
> Should prefer pre-increment over post-increment if you don't need the original value.
Right, fixed.
================
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:
> 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.
================
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
----------------
Edwin Vane wrote:
> `constructors redeclarations` -> `redeclarations of \p Ctor`.
Done.
================
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;
+ }
+
----------------
Edwin Vane wrote:
> Shouldn't this test be one of the first things done?
The loop before has an early exit in case we can't modify one of the references to the parameter that need to be changed.
Doing this check earlier means we will potentially trigger a rejected change when it's not needed in the case one of the parameter couldn't be changed.
================
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
----------------
Edwin Vane wrote:
> `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.
Done.
================
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
----------------
Edwin Vane wrote:
> `instead of taking a` -> `instead of by`. Remove the comma too.
Done.
================
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.
----------------
Edwin Vane wrote:
> `That way the compiler takes` -> `This transformation allows the compiler to take`
Done.
================
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
----------------
Edwin Vane wrote:
> `This` -> `The transformation`
> `pass` -> `passes`
>
> Remove parentheses and `assuming` -> `and assumes`.
Done, done, done.
================
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!
+
----------------
Edwin Vane wrote:
> I'd reword to `Contributions that handle other situations are welcome!`.
Done.
================
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.
----------------
Edwin Vane wrote:
> `make the insertion` -> `add the include directive`.
Done.
================
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.
+
----------------
Edwin Vane wrote:
> `The situation` -> `A situation`. Can't be guaranteed this is the only situation:)
>
> Remove the last sentence.
Done, done.
================
Comment at: test/cpp11-migrate/PassByValue/basic.h:15
@@ +14,3 @@
+};
+
+struct UnmodifiableClass {
----------------
Edwin Vane wrote:
> How about the case of a struct with a user-defined move constructor?
Test added in basic.cpp.
================
Comment at: test/cpp11-migrate/PassByValue/basic.h:16
@@ +15,3 @@
+
+struct UnmodifiableClass {
+ UnmodifiableClass(const Movable &M);
----------------
Edwin Vane wrote:
> Can you say why this isn't modifiable?
Done.
================
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 {
----------------
Edwin Vane wrote:
> Have a CHECK: here that ensures the macro body wasn't changed. By the way, what part of the transform prevents this from happening?
Done.
The function `Lexer::makeFileCharRange()` used in the PassByValueAction.cpp takes care of the inconveniences with macros (see http://clang.llvm.org/doxygen/classclang_1_1Lexer.html#a7e7f08993261441a8d83d9253ca53859).
================
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:
> 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;
};
http://llvm-reviews.chandlerc.com/D1342
BRANCH
pass-by-value
ARCANIST PROJECT
clang-tools-extra
More information about the cfe-commits
mailing list