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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
