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

[Edwin Vane]

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

  Don't forget to update MigratorUsage.rst.

  I really like the organization of the matchers. There are some ways of doing things here that could be used to improve UseAuto as well.


================
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
----------------
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).

================
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
----------------
Perhaps an extra added note about the difference in semantics between auto_ptr and unique_ptr to explain why ownership transfer requries std::move.

================
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.
----------------
You can remove the `(s)`.

================
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().
----------------
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.

================
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
----------------
I'd just say lvalue instead of non-rvalue.

================
Comment at: docs/ReplaceAutoPtrTransform.rst:45
@@ +44,3 @@
+  even coming from code that can't be migrated, it is not an issue since
+  ``std::unique_ptr`` can be constructed from a non-lvalue ``std::auto_ptr``.
+
----------------
Again, avoid negatives to make easier reading and just say rvalue here.

================
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
----------------
can't that can't.

================
Comment at: docs/ReplaceAutoPtrTransform.rst:38
@@ +37,3 @@
+  transform because declarations in headers will stay unchanged while the code
+  using them will be changed.
+
----------------
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.

================
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> &);
----------------
Can you add a test for passing auto_ptr by value to a func?

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

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/Inputs/basic.h:4
@@ +3,3 @@
+
+#include <memory>
+
----------------
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.

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

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

================
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>;
----------------
Log a bug so we can track it.

================
Comment at: test/cpp11-migrate/ReplaceAutoPtr/template_fail.cpp:8
@@ +7,3 @@
+
+// Fail to modify when the template is never instanciated.
+//
----------------
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.

================
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()));
+
----------------
Could we have a test where

  std::auto_ptr<int> a = some_func_returning_auto_ptr_by_value();


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