Address review comments.
Hi revane,
http://llvm-reviews.chandlerc.com/D1074
CHANGE SINCE LAST DIFF
http://llvm-reviews.chandlerc.com/D1074?vs=2623&id=2650#toc
Files:
cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtr.h
cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrActions.cpp
cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrActions.h
cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrMatchers.cpp
cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrMatchers.h
docs/MigratorUsage.rst
docs/ReplaceAutoPtrTransform.rst
test/cpp11-migrate/ReplaceAutoPtr/Inputs/basic.h
test/cpp11-migrate/ReplaceAutoPtr/Inputs/memory_stub.h
test/cpp11-migrate/ReplaceAutoPtr/basic.cpp
test/cpp11-migrate/ReplaceAutoPtr/move.cpp
test/cpp11-migrate/ReplaceAutoPtr/std_inline_namespace.cpp
test/cpp11-migrate/ReplaceAutoPtr/template_fail.cpp
Index: cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtr.h
===================================================================
--- cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtr.h
+++ cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtr.h
@@ -12,18 +12,23 @@
/// class.
///
//===----------------------------------------------------------------------===//
-#ifndef LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_H
-#define LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_H
+#ifndef CPP11_MIGRATE_REPLACE_AUTO_PTR_H
+#define CPP11_MIGRATE_REPLACE_AUTO_PTR_H
#include "Core/Transform.h"
#include "llvm/Support/Compiler.h"
/// \brief Subclass of Transform that transforms the deprecated \c std::auto_ptr
/// into the C++11 \c std::unique_ptr.
///
/// 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
-/// explicit call to \c std::move().
+/// transformed. \c std::auto_ptr provides two ways to transfer the ownership,
+/// the copy-constructor and the assignment operator. Unlike most classes theses
+/// operations do not 'copy' the resource but they 'steal' it.
+/// \c std::unique_ptr uses move semantics instead, which makes the intent of
+/// transferring the resource explicit. This difference between the two smart
+/// pointers requires to wrap the copy-ctor and assign-operator with
+/// \c std::move().
///
/// For example, given:
/// \code
@@ -46,4 +51,4 @@
const std::vector<std::string> &SourcePaths) LLVM_OVERRIDE;
};
-#endif // LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_H
+#endif // CPP11_MIGRATE_REPLACE_AUTO_PTR_H
Index: cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrActions.cpp
===================================================================
--- cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrActions.cpp
+++ cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrActions.cpp
@@ -8,7 +8,7 @@
//===----------------------------------------------------------------------===//
///
/// \file
-/// \brief This file contains the definition(s) of the ASTMatcher callback(s)
+/// \brief This file contains the definition of the ASTMatcher callback
/// for the ReplaceAutoPtr transform.
///
//===----------------------------------------------------------------------===//
Index: cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrActions.h
===================================================================
--- cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrActions.h
+++ cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrActions.h
@@ -8,12 +8,12 @@
//===----------------------------------------------------------------------===//
///
/// \file
-/// \brief This file contains the declaration(s) of the ASTMatcher callback(s)
+/// \brief This file contains the declaration of the ASTMatcher callback
/// for the ReplaceAutoPtr transform.
///
//===----------------------------------------------------------------------===//
-#ifndef LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_ACTIONS_H
-#define LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_ACTIONS_H
+#ifndef CPP11_MIGRATE_REPLACE_AUTO_PTR_ACTIONS_H
+#define CPP11_MIGRATE_REPLACE_AUTO_PTR_ACTIONS_H
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Tooling/Refactoring.h"
@@ -99,4 +99,4 @@
const Transform &Owner;
};
-#endif // LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_ACTIONS_H
+#endif // CPP11_MIGRATE_REPLACE_AUTO_PTR_ACTIONS_H
Index: cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrMatchers.cpp
===================================================================
--- cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrMatchers.cpp
+++ cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrMatchers.cpp
@@ -79,10 +79,10 @@
static TypeMatcher AutoPtrType = qualType(hasDeclaration(AutoPtrDecl));
-/// \brief Matcher that finds expressions that are candidates to be wrapped with
-/// \c std::move().
-///
-/// Bind the id \c AutoPtrOwnershipTransferId to the expression.
+// Matcher that finds expressions that are candidates to be wrapped with
+// 'std::move()'.
+//
+// Binds the id \c AutoPtrOwnershipTransferId to the expression.
static StatementMatcher MovableArgumentMatcher = expr(
allOf(isLValue(), hasType(AutoPtrType))).bind(AutoPtrOwnershipTransferId);
Index: cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrMatchers.h
===================================================================
--- cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrMatchers.h
+++ cpp11-migrate/ReplaceAutoPtr/ReplaceAutoPtrMatchers.h
@@ -12,8 +12,8 @@
/// and names for bound nodes found by AST matchers.
///
//===----------------------------------------------------------------------===//
-#ifndef LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_MATCHERS_H
-#define LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_MATCHERS_H
+#ifndef CPP11_MIGRATE_REPLACE_AUTO_PTR_MATCHERS_H
+#define CPP11_MIGRATE_REPLACE_AUTO_PTR_MATCHERS_H
#include "clang/ASTMatchers/ASTMatchers.h"
@@ -60,4 +60,4 @@
/// \endcode
clang::ast_matchers::StatementMatcher makeTransferOwnershipExprMatcher();
-#endif // LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_REPLACE_AUTO_PTR_MATCHERS_H
+#endif // CPP11_MIGRATE_REPLACE_AUTO_PTR_MATCHERS_H
Index: docs/MigratorUsage.rst
===================================================================
--- docs/MigratorUsage.rst
+++ docs/MigratorUsage.rst
@@ -126,3 +126,9 @@
projects that use such macros to maintain build compatibility with non-C++11
code.
+.. option:: -replace-auto_ptr
+
+ Replace ``std::auto_ptr`` (deprecated in C++11) by ``std::unique_ptr`` and
+ wrap calls to the copy constructor and assignment operator with
+ ``std::move()``.
+ See :doc:`ReplaceAutoPtrTransform`.
Index: docs/ReplaceAutoPtrTransform.rst
===================================================================
--- docs/ReplaceAutoPtrTransform.rst
+++ docs/ReplaceAutoPtrTransform.rst
@@ -33,27 +33,40 @@
Known Limitations
=================
-* Until headers modification is fully functional it is dangerous to use this
- transform because declarations in headers will stay unchanged while the code
- using them will be changed.
+* If headers modification is not activated or if a header is not allowed to be
+ changed this transform will produce broken code (compilation error), where the
+ the headers' code will stay unchanged while the code using them will be
+ changed.
-* 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
- and usually ``std::auto_ptr`` are returned by value. When returned by value,
- 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``.
+* Client code that declares a reference to an ``std::auto_ptr`` coming from code
+ that can't be migrated (such as a header coming from a 3\ :sup:`rd` party
+ library) will produce a compilation error after migration. This is because the
+ type of the reference will be changed to ``std::unique_ptr`` but the type
+ returned by the library won't change, binding a reference to
+ ``std::unique_ptr`` from an ``std::auto_ptr``. This pattern doesn't make much
+ sense and usually ``std::auto_ptr`` are stored by value (otherwise what is the
+ point in using them instead of a reference or a pointer?).
+ .. code-block:: c++
-.. code-block:: c++
+ // <3rd-party header...>
+ std::auto_ptr<int> get_value();
+ const std::auto_ptr<int> & get_ref();
+
+ // <calling code (with migration)...>
+ -std::auto_ptr<int> a(get_value());
+ +std::unique_ptr<int> a(get_value()); // ok, unique_ptr constructed from auto_ptr
+
+ -const std::auto_ptr<int> & p = get_ptr();
+ +const std::unique_ptr<int> & p = get_ptr(); // won't compile
+
+* Non-instantiated templates aren't modified.
- // <3rd-party header...>
- std::auto_ptr<int> get_value();
- const std::auto_ptr<int> & get_ref();
+ .. code-block:: c++
- // <calling code (with migration)...>
- -std::auto_ptr<int> a(get_value());
- +std::unique_ptr<int> a(get_value()); // ok, unique_ptr constructed from auto_ptr
+ template <typename X>
+ void f() {
+ std::auto_ptr<X> p;
+ }
- -const std::auto_ptr<int> & p = get_ptr();
- +const std::unique_ptr<int> & p = get_ptr(); // won't compile
+ // only 'f<int>()' (or similar) will trigger the replacement
Index: test/cpp11-migrate/ReplaceAutoPtr/Inputs/basic.h
===================================================================
--- test/cpp11-migrate/ReplaceAutoPtr/Inputs/basic.h
+++ test/cpp11-migrate/ReplaceAutoPtr/Inputs/basic.h
@@ -1,7 +1,7 @@
#ifndef INPUTS_BASIC_H
#define INPUTS_BASIC_H
-#include <memory>
+#include "memory_stub.h"
// Instrumentation for auto_ptr_ref test
// @{
@@ -16,8 +16,10 @@
// CHECK: std::unique_ptr<char> f_5()
// Test function parameters
-void f_6(const std::auto_ptr<int> &);
-// CHECK: void f_6(const std::unique_ptr<int> &);
+void f_6(std::auto_ptr<int>);
+// CHECK: void f_6(std::unique_ptr<int>);
+void f_7(const std::auto_ptr<int> &);
+// CHECK: void f_7(const std::unique_ptr<int> &);
// Test on record type fields
struct A {
@@ -28,7 +30,7 @@
// CHECK: typedef std::unique_ptr<int> int_ptr_type;
};
-// Test template WITH instanciation
+// Test template WITH instantiation
template <typename T> struct B {
typedef typename std::auto_ptr<T> created_type;
// CHECK: typedef typename std::unique_ptr<T> created_type;
Index: test/cpp11-migrate/ReplaceAutoPtr/Inputs/memory_stub.h
===================================================================
--- /dev/null
+++ test/cpp11-migrate/ReplaceAutoPtr/Inputs/memory_stub.h
@@ -0,0 +1,61 @@
+//===-----------------------------------------------------------*- C++ -*--===//
+//
+// This file contains a shell implementation of the 'auto_ptr' type from the
+// standard library. This shell aims to support the variations between standard
+// library implementations.
+//
+// Variations for how 'auto_ptr' is presented:
+// 1. Defined directly in namespace std
+// 2. Use a versioned inline namespace in std (default on libc++).
+//
+// Use the preprocessor to define USE_INLINE_NAMESPACE=1 and use the second
+// variation.
+//
+//===----------------------------------------------------------------------===//
+
+namespace std {
+
+#if USE_INLINE_NAMESPACE
+inline namespace _1 {
+#endif
+
+template <class Y> struct auto_ptr_ref {
+ Y *y_;
+};
+
+template <class X> class auto_ptr {
+public:
+ typedef X element_type;
+ // D.10.1.1 construct/copy/destroy:
+ explicit auto_ptr(X *p = 0) throw() {}
+ auto_ptr(auto_ptr &) throw() {}
+ template <class Y> auto_ptr(auto_ptr<Y> &) throw() {}
+ auto_ptr &operator=(auto_ptr &) throw() { return *this; }
+ template <class Y> auto_ptr &operator=(auto_ptr<Y> &) throw() {
+ return *this;
+ }
+ auto_ptr &operator=(auto_ptr_ref<X> r) throw() { return *this; }
+ ~auto_ptr() throw() {}
+ // D.10.1.3 conversions:
+ auto_ptr(auto_ptr_ref<X> r) throw() : x_(r.y_) {}
+ template <class Y> operator auto_ptr_ref<Y>() throw() {
+ auto_ptr_ref<Y> r;
+ r.y_ = x_;
+ return r;
+ }
+ template <class Y> operator auto_ptr<Y>() throw() { return auto_ptr<Y>(x_); }
+
+private:
+ X *x_;
+};
+
+template <> class auto_ptr<void> {
+public:
+ typedef void element_type;
+};
+
+#if USE_INLINE_NAMESPACE
+} // namespace _1
+#endif
+
+} // end namespace std
Index: test/cpp11-migrate/ReplaceAutoPtr/basic.cpp
===================================================================
--- test/cpp11-migrate/ReplaceAutoPtr/basic.cpp
+++ test/cpp11-migrate/ReplaceAutoPtr/basic.cpp
@@ -1,10 +1,24 @@
// RUN: mkdir -p %T/Inputs
+//
+// Without inline namespace:
+//
// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/basic.h > %T/Inputs/basic.h
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/memory_stub.h > %T/Inputs/memory_stub.h
// RUN: cpp11-migrate -headers -include=%T -replace-auto_ptr %t.cpp -- \
// RUN: -std=c++11 -I %T
// RUN: FileCheck -input-file=%t.cpp %s
// RUN: FileCheck -input-file=%T/Inputs/basic.h %S/Inputs/basic.h
+//
+// With inline namespace:
+//
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/basic.h > %T/Inputs/basic.h
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/memory_stub.h > %T/Inputs/memory_stub.h
+// RUN: cpp11-migrate -headers -include=%T -replace-auto_ptr %t.cpp -- \
+// RUN: -DUSE_INLINE_NAMESPACE=1 -std=c++11 -I %T
+// RUN: FileCheck -input-file=%t.cpp %s
+// RUN: FileCheck -input-file=%T/Inputs/basic.h %S/Inputs/basic.h
#include "Inputs/basic.h"
@@ -61,7 +75,7 @@
std::auto_ptr<void> n;
// CHECK: std::unique_ptr<void> n;
- // Test template WITH instanciation (instanciation)
+ // Test template WITH instantiation (instantiation)
B<double> o;
std::auto_ptr<double> p(o.create());
// CHECK: std::unique_ptr<double> p(o.create());
@@ -122,7 +136,7 @@
}
// Test that non-std auto_ptr aren't replaced
-void f_7() {
+void f_8() {
ns_2::auto_ptr<char> a;
// CHECK: ns_2::auto_ptr<char> a;
using namespace ns_2;
Index: test/cpp11-migrate/ReplaceAutoPtr/move.cpp
===================================================================
--- test/cpp11-migrate/ReplaceAutoPtr/move.cpp
+++ test/cpp11-migrate/ReplaceAutoPtr/move.cpp
@@ -1,10 +1,23 @@
+// Without inline namespace:
+//
// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: cpp11-migrate -replace-auto_ptr %t.cpp -- -std=c++11
+// RUN: cpp11-migrate -replace-auto_ptr %t.cpp -- -I %S/Inputs std=c++11
+// RUN: FileCheck -input-file=%t.cpp %s
+//
+// With inline namespace:
+//
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: cpp11-migrate -replace-auto_ptr %t.cpp -- -I %S/Inputs std=c++11 \
+// RUN: -DUSE_INLINE_NAMESPACE=1
// RUN: FileCheck -input-file=%t.cpp %s
-#include <memory>
+#include "memory_stub.h"
-void take_ownership_fn(std::auto_ptr<int> x);
+void takes_ownership_fn(std::auto_ptr<int> x);
+// CHECK: void takes_ownership_fn(std::unique_ptr<int> x);
+
+std::auto_ptr<int> get_by_value();
+// CHECK: std::unique_ptr<int> get_by_value();
class Wrapper {
public:
@@ -27,10 +40,10 @@
// Test that 'std::move()' is inserted when call to the
// copy-constructor are made.
- take_ownership_fn(c);
- // CHECK: take_ownership_fn(std::move(c));
- take_ownership_fn(wrapper_a.get_wrapped());
- // CHECK: take_ownership_fn(std::move(wrapper_a.get_wrapped()));
+ takes_ownership_fn(c);
+ // CHECK: takes_ownership_fn(std::move(c));
+ takes_ownership_fn(wrapper_a.get_wrapped());
+ // CHECK: takes_ownership_fn(std::move(wrapper_a.get_wrapped()));
std::auto_ptr<int> d[] = { std::auto_ptr<int>(new int(1)),
std::auto_ptr<int>(new int(2)) };
@@ -44,4 +57,7 @@
f = std::auto_ptr<int>(new int(0));
// CHECK: std::unique_ptr<int> f;
// CHECK-NEXT: f = std::unique_ptr<int>(new int(0));
+
+ std::auto_ptr<int> g = get_by_value();
+ // CHECK: std::unique_ptr<int> g = get_by_value();
}
Index: test/cpp11-migrate/ReplaceAutoPtr/std_inline_namespace.cpp
===================================================================
--- test/cpp11-migrate/ReplaceAutoPtr/std_inline_namespace.cpp
+++ /dev/null
@@ -1,23 +0,0 @@
-// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: cpp11-migrate -replace-auto_ptr %t.cpp -- -std=c++11
-// RUN: FileCheck -input-file=%t.cpp %s
-
-namespace std {
-
-// Check that even when declared in an inline namespace auto_ptr will be found.
-// libc++ use an inline versioned namespace to declare standard library types.
-//
-// Note that libstdc++ seems to have the same possibility but apparently off by
-// default.
-inline namespace __1 {
-
-template <typename T> class auto_ptr {};
-
-} // namespace '__1' ends here
-using namespace __1;
-
-} // namespace 'std' ends here
-
-
-std::auto_ptr<int> a;
-// CHECK: std::unique_ptr<int> a;
Index: test/cpp11-migrate/ReplaceAutoPtr/template_fail.cpp
===================================================================
--- test/cpp11-migrate/ReplaceAutoPtr/template_fail.cpp
+++ test/cpp11-migrate/ReplaceAutoPtr/template_fail.cpp
@@ -1,11 +1,21 @@
+// XFAIL: *
+//
+// Without inline namespace:
+//
// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: cpp11-migrate -replace-auto_ptr %t.cpp -- -std=c++11
+// RUN: cpp11-migrate -replace-auto_ptr %t.cpp -- -I %S/Inputs std=c++11
+// RUN: FileCheck -input-file=%t.cpp %s
+//
+// With inline namespace:
+//
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: cpp11-migrate -replace-auto_ptr %t.cpp -- -I %S/Inputs std=c++11 \
+// RUN: -DUSE_INLINE_NAMESPACE=1
// RUN: FileCheck -input-file=%t.cpp %s
-// XFAIL: *
-#include <memory>
+#include "memory_stub.h"
-// Fail to modify when the template is never instanciated.
+// Fail to modify when the template is never instantiated.
//
// This might not be an issue. If it's never used it doesn't really matter if
// it's changed or not. If it's a header and one of the source use it, then it
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
