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