[PATCH] cpp11-migrate: Add Replace-AutoPtr Transform

[Guillaume Papin]

  > Have you tried running make doxygen in the docs folder to make sure there are no warnings?

  I just ran Doxygen, no warnings.

  > Don't forget to update MigratorUsage.rst.

  Thanks, I didn't notice this one, usage added.


================
Comment at: cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtr.h:15
@@ +14,3 @@
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_H
+#define LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_H
----------------
Edwin Vane wrote:
> Any new files we've been adding have been using the much shorter header guard style of:
> 
> CPP11_MIGRATE_<filename>
> 
> I haven't got around to fixing all the other files yet (CM-82).
Fixed.

================
Comment at: cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtr.h:24
@@ +23,3 @@
+///
+/// Note that both the \c std::auto_ptr type and the transfer of ownership are
+/// transformed. The latter requires to wrap the assigned expression with an
----------------
Edwin Vane wrote:
> Perhaps an extra added note about the difference in semantics between auto_ptr and unique_ptr to explain why ownership transfer requries std::move.
Done.

================
Comment at: cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrActions.h:11
@@ +10,3 @@
+/// \file
+/// \brief This file contains the declaration(s) of the ASTMatcher callback(s)
+/// for the ReplaceAutoPtr transform.
----------------
Edwin Vane wrote:
> You can remove the `(s)`.
Done.

================
Comment at: cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrMatchers.cpp:82
@@ +81,3 @@
+
+/// \brief Matcher that finds expressions that are candidates to be wrapped with
+/// \c std::move().
----------------
Edwin Vane wrote:
> For private implementations you don't need to follow doxygen style for documentation. LLVM at the least requires public interfaces to be documented. Private interfaces in headers can be documented if you like but stuff inside source files doesn't need doxygen style.
Slightly modified.

================
Comment at: docs/ReplaceAutoPtrTransform.rst:38
@@ +37,3 @@
+  transform because declarations in headers will stay unchanged while the code
+  using them will be changed.
+
----------------
Edwin Vane wrote:
> Since you mention third party libraries below, you might indicate that such libraries whose headers we're not allowed to change will also cause a problem even when header transforms work properly.
I rephrased this limitation.

================
Comment at: docs/ReplaceAutoPtrTransform.rst:40
@@ +39,3 @@
+
+* Code that can't that can't be migrated, such as 3\ :sup:`rd` party libraries,
+  returning non-rvalue references to ``std::auto_ptr`` or ``const
----------------
Edwin Vane wrote:
> can't that can't.
Fixed.

================
Comment at: docs/ReplaceAutoPtrTransform.rst:41
@@ +40,3 @@
+* Code that can't that can't be migrated, such as 3\ :sup:`rd` party libraries,
+  returning non-rvalue references to ``std::auto_ptr`` or ``const
+  std::auto_ptr`` will produce wrong code. Theses patterns don't make much sense
----------------
Edwin Vane wrote:
> I'd just say lvalue instead of non-rvalue.
I rewrote this limitation.
My brain was a bit confused about xvalue  that's why I use the negative form twice, sorry about that.

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/Inputs/basic.h:4
@@ +3,3 @@
+
+#include <memory>
+
----------------
Edwin Vane wrote:
> I'd prefer you didn't include a standard header here and that you define your own stub header for the purposes of the test. Otherwise this test might start failing on systems due to weirdnesses in the standard headers whereas we'd like our tests to test the quality of OUR code.
> 
> As you've discovered, there are different ways std::auto_ptr can be revealed by the various standard libs out there so I recommend you make several stub headers to test each method. We can use lit to control which header gets included and just run the test multiple times.
Done!

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/Inputs/basic.h:19
@@ +18,3 @@
+// Test function parameters
+void f_6(const std::auto_ptr<int> &);
+// CHECK: void f_6(const std::unique_ptr<int> &);
----------------
Edwin Vane wrote:
> Can you add a test for passing auto_ptr by value to a func?
I added a test where the function takes the argument by value. But the tests testing `std::move()` are in move.cpp if that's what you wanted (and taking by value is tested there).

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/Inputs/basic.h:31
@@ +30,3 @@
+
+// Test template WITH instanciation
+template <typename T> struct B {
----------------
Edwin Vane wrote:
> Instantiation.
Fixed.

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/basic.cpp:19
@@ +18,3 @@
+  std :: auto_ptr < char > c(new char());
+  // CHECK: std :: unique_ptr < char > c(new char());
+
----------------
Edwin Vane wrote:
> FYI: we don't care too much about spaces. Sometimes we can't help but make whitespace changes. Using libformat after transforms will help eventually.
Yeah, I wasn't too worried about this, in this case I just want to check that we are modifying only the "auto_ptr token", there is no need to change more.

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/basic.cpp:64
@@ +63,3 @@
+
+  // Test template WITH instanciation (instanciation)
+  B<double> o;
----------------
Edwin Vane wrote:
> typo here again.
Fixed.

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/move.cpp:33
@@ +32,3 @@
+  take_ownership_fn(wrapper_a.get_wrapped());
+  // CHECK: take_ownership_fn(std::move(wrapper_a.get_wrapped()));
+
----------------
Edwin Vane wrote:
> Could we have a test where
> 
>   std::auto_ptr<int> a = some_func_returning_auto_ptr_by_value();
Added.

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/template_fail.cpp:8
@@ +7,3 @@
+
+// Fail to modify when the template is never instanciated.
+//
----------------
Edwin Vane wrote:
> Several other transforms face this problem too but it's worth noting it in the RST docs for this transform anyway. The other transforms should be updated similarly one day.
Done.

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/template_fail.cpp:21
@@ +20,2 @@
+template <typename T> using aaaaaaaa = auto_ptr<T>;
+// CHECK: template <typename T> using aaaaaaaa = unique_ptr<T>;
----------------
Edwin Vane wrote:
> Log a bug so we can track it.
Done!


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