Movable but not copyable bug?
I had posted this on the user's list but I think this should be brought to the attention of the developers. The following code contains a templated copy constructible Boost.Move enabled class (Bar) with a single non-copy constructible Boost.Enabled member (Foo). If Bar's copy constructor is not explicitly defined then the code compiles with both g++ 4.8.2 and VS2005, else if it's explicitly defined then it fails to compile on g++ 4.8.2 I think this is due to the fact that g++ 4.8.2 erroneously instantiates Bar's default copy constructor in the latter case. Is this a Boost.Move bug or a gcc bug? #include <boost/move/core.hpp> #include <boost/move/utility.hpp> struct Foo { Foo(int x, int y) {} Foo(BOOST_RV_REF(Foo) rhs) {} private: //Purposefully declared and not defined. Foo & operator=(Foo); private: BOOST_MOVABLE_BUT_NOT_COPYABLE(Foo) }; template <typename T> struct Bar { Bar() : f(0, 1) {} // (1) //Bar(Bar const & rhs) //: f(rhs.f) //{} Bar(BOOST_RV_REF(Bar) rhs) : f(::boost::move(rhs.f)) {} private: //Purposefully declared and not defined. Bar & operator=(Bar); private: BOOST_COPYABLE_AND_MOVABLE(Bar) private: T f; }; Foo f1() { return Foo(0, 1); } Bar<Foo> f2() { return Bar<Foo>(); } int main() { // Foo f( f1() ); Bar<Foo> b(( Bar<Foo>() )); // Bar<Foo> b1; // Bar<Foo> b2(b1); return 0; }
El 25/08/2014 2:29, Mostafa escribió:
I had posted this on the user's list but I think this should be brought to the attention of the developers. The following code contains a templated copy constructible Boost.Move enabled class (Bar) with a single non-copy constructible Boost.Enabled member (Foo). If Bar's copy constructor is not explicitly defined then the code compiles with both g++ 4.8.2 and VS2005, else if it's explicitly defined then it fails to compile on g++ 4.8.2 I think this is due to the fact that g++ 4.8.2 erroneously instantiates Bar's default copy constructor in the latter case. Is this a Boost.Move bug or a gcc bug?
The copy constructor shall not be defined. If the error comes when returning by value, use BOOST_MOVE_RET: http://www.boost.org/doc/libs/1_56_0/doc/html/move/move_return.html Otherwise, please be a bit more explicit about the error, which operations, fails, etc. I also noted that you've disabled the assignment operator taking by value. I have no idea on what can happen, as Boost.Move disables also the copy assignment taking by non-const reference. Best, Ion
On Mon, 25 Aug 2014 03:23:59 -0700, Ion Gaztañaga <igaztanaga@gmail.com> wrote: [snip]
I also noted that you've disabled the assignment operator taking by value. I have no idea on what can happen, as Boost.Move disables also the copy assignment taking by non-const reference.
Does that mean that when using BOOST_MOVABLE_BUT_NOT_COPYABLE clients should not delete the copy constructor or the copy assignment operator? If so, that should be documented. Also, can BOOST_COPYABLE_AND_MOVABLE be used with the copy-and-swap idiom?
El 25/08/2014 18:08, Mostafa escribió:
On Mon, 25 Aug 2014 03:23:59 -0700, Ion Gaztañaga <igaztanaga@gmail.com> wrote:
[snip]
I also noted that you've disabled the assignment operator taking by value. I have no idea on what can happen, as Boost.Move disables also the copy assignment taking by non-const reference.
Does that mean that when using BOOST_MOVABLE_BUT_NOT_COPYABLE clients should not delete the copy constructor or the copy assignment operator? If so, that should be documented.
I think it's documented: http://www.boost.org/doc/libs/1_56_0/doc/html/move/movable_only_classes.html http://www.boost.org/doc/libs/1_56_0/doc/html/BOOST_MOVABLE_BUT_NOT_COPYABLE...
Also, can BOOST_COPYABLE_AND_MOVABLE be used with the copy-and-swap idiom?
I haven't tried, but I think it will work. Try avoiding a BOOST_COPY_ASSIGN-based assignment shown here http://www.boost.org/doc/libs/1_56_0/doc/html/move/implementing_movable_clas... and define your assignment taking by value. I hope it helps, Ion
On Mon, 25 Aug 2014 14:01:18 -0700, Ion Gaztañaga <igaztanaga@gmail.com> wrote:
El 25/08/2014 18:08, Mostafa escribió:
On Mon, 25 Aug 2014 03:23:59 -0700, Ion Gaztañaga <igaztanaga@gmail.com> wrote:
[snip]
I also noted that you've disabled the assignment operator taking by value. I have no idea on what can happen, as Boost.Move disables also the copy assignment taking by non-const reference.
Does that mean that when using BOOST_MOVABLE_BUT_NOT_COPYABLE clients should not delete the copy constructor or the copy assignment operator? If so, that should be documented.
I think it's documented:
http://www.boost.org/doc/libs/1_56_0/doc/html/move/movable_only_classes.html
http://www.boost.org/doc/libs/1_56_0/doc/html/BOOST_MOVABLE_BUT_NOT_COPYABLE...
Ah, ok , thanks. That information is indeed documented in the second link, but not the first. I think it should also be documented in the first link, since that page presents a formulaic instruction on how to use the library. From the first link: To write a movable but not copyable type in portable syntax, you need to follow these simple steps: - Put the following macro in the private section: BOOST_MOVABLE_BUT_NOT_COPYABLE(classname) - Write a move constructor and a move assignment taking the parameter as BOOST_RV_REF(classname) Having BOOST_MOVABLE_BUT_NOT_COPYABLE usage requirements spread across two pages is confusing IMO. I suggest adding the following to the above documentation snippet: - Do not delete the copy constructor. (In C++03 do not declare it and leave it undefined.) - Do not delete the copy assignment operator. (In C++03 do not declare it and leave it undefined.) Thanks, Mostafa
On Mon, 25 Aug 2014 03:23:59 -0700, Ion Gaztañaga <igaztanaga@gmail.com> wrote:
El 25/08/2014 2:29, Mostafa escribió:
I had posted this on the user's list but I think this should be brought to the attention of the developers. The following code contains a templated copy constructible Boost.Move enabled class (Bar) with a single non-copy constructible Boost.Enabled member (Foo). If Bar's copy constructor is not explicitly defined then the code compiles with both g++ 4.8.2 and VS2005, else if it's explicitly defined then it fails to compile on g++ 4.8.2 I think this is due to the fact that g++ 4.8.2 erroneously instantiates Bar's default copy constructor in the latter case. Is this a Boost.Move bug or a gcc bug?
The copy constructor shall not be defined.
???
Otherwise, please be a bit more explicit about the error, which operations, fails, etc.
Here's the updated code with error messages. Note, that on g++ 4.8.2 if (1) is commented out then it compiles fine but if uncommented then the error messages that follow the code are produced. In the latter case, the error messages are due to (2). Note also that (1) is the copy constructor for the movable *and* copyable type Bar which has a single member of type Foo that is movable but *not* copyable. Also note that Bar is a template class. //Start Code #include <boost/move/core.hpp> #include <boost/move/utility.hpp> struct Foo { Foo(int x, int y) {} Foo(BOOST_RV_REF(Foo) rhs) {} private: //Purposefully declared and not defined. //Foo & operator=(Foo); private: BOOST_MOVABLE_BUT_NOT_COPYABLE(Foo) }; template <typename T> struct Bar { Bar() : f(0, 1) {} // (1) Bar(Bar const & rhs) : f(rhs.f) {} Bar(BOOST_RV_REF(Bar) rhs) : f(::boost::move(rhs.f)) {} private: //Purposefully declared and not defined. Bar & operator=(Bar); private: BOOST_COPYABLE_AND_MOVABLE(Bar) private: T f; }; int main() { // (2) Bar<Foo> b(( Bar<Foo>() )); return 0; } //End Code //Start Error Messages boost_move_default_copy_ctor.cc: In instantiation of ‘Bar<T>::Bar(const Bar<T>&) [with T = Foo]’: boost_move_default_copy_ctor.cc:47:28: required from here boost_move_default_copy_ctor.cc:28:12: error: invalid user-defined conversion from ‘const Foo’ to ‘boost::rv<Foo>&’ [-fpermissive] : f(rhs.f) ^ In file included from boost_move_default_copy_ctor.cc:1:0: /home/HPC/boost_installed/v1_56_0/include/boost/move/core.hpp:219:7: note: candidate is: Foo::operator boost::rv<Foo>&() <near match> operator ::boost::rv<TYPE>&() \ ^ boost_move_default_copy_ctor.cc:16:3: note: in expansion of macro ‘BOOST_MOVABLE_BUT_NOT_COPYABLE’ BOOST_MOVABLE_BUT_NOT_COPYABLE(Foo) ^ /home/HPC/boost_installed/v1_56_0/include/boost/move/core.hpp:219:7: note: no known conversion for implicit ‘this’ parameter from ‘const Foo*’ to ‘Foo*’ operator ::boost::rv<TYPE>&() \ ^ boost_move_default_copy_ctor.cc:16:3: note: in expansion of macro ‘BOOST_MOVABLE_BUT_NOT_COPYABLE’ BOOST_MOVABLE_BUT_NOT_COPYABLE(Foo) ^ boost_move_default_copy_ctor.cc:28:12: error: passing ‘const Foo’ as ‘this’ argument of ‘Foo::operator boost::rv<Foo>&()’ discards qualifiers [-fpermissive] : f(rhs.f) ^ //End Error Messages
El 25/08/2014 18:09, Mostafa escribió:
On Mon, 25 Aug 2014 03:23:59 -0700, Ion Gaztañaga <igaztanaga@gmail.com> wrote:
El 25/08/2014 2:29, Mostafa escribió:
I had posted this on the user's list but I think this should be brought to the attention of the developers. The following code contains a templated copy constructible Boost.Move enabled class (Bar) with a single non-copy constructible Boost.Enabled member (Foo). If Bar's copy constructor is not explicitly defined then the code compiles with both g++ 4.8.2 and VS2005, else if it's explicitly defined then it fails to compile on g++ 4.8.2 I think this is due to the fact that g++ 4.8.2 erroneously instantiates Bar's default copy constructor in the latter case. Is this a Boost.Move bug or a gcc bug?
The copy constructor shall not be defined.
???
If you declare a class as BOOST_MOVABLE_BUT_NOT_COPYABLE(Foo) you must not declare the assignment operator. Follow the steps explained here: http://www.boost.org/doc/libs/1_56_0/doc/html/move/movable_only_classes.html
Here's the updated code with error messages. Note, that on g++ 4.8.2 if (1) is commented out then it compiles fine but if uncommented then the error messages that follow the code are produced. In the latter case, the error messages are due to (2). Note also that (1) is the copy constructor for the movable *and* copyable type Bar which has a single member of type Foo that is movable but *not* copyable. Also note that Bar is a template class.
If Foo is not copyable, then you have a logic error in your code: Bar(Bar const & rhs) : f(rhs.f) <---- Tries to COPY Foo {} If this copy constructor is instantiated, and in many compilers Bar<Foo> b(( Bar<Foo>() )); requires the copy constructor, then you get a compilation error. Just try this equivalent (but more inefficient) code: Bar<Foo> a; Bar<Foo> b(a); Then the copy constructor of Bar<Foo> is called and tries to COPY Foo. As Foo is noncopyable, a compilation error is triggered. Maybe you should design your code in C++11 and port it to Boost.Move, you might get better compiler help. Best, Ion
Ion Gaztañaga wrote:
Maybe you should design your code in C++11 and port it to Boost.Move, you might get better compiler help.
His code compiles in C++11, as is.
If this copy constructor is instantiated, and in many compilers
Bar<Foo> b(( Bar<Foo>() ));
requires the copy constructor, then you get a compilation error. Just try this equivalent (but more inefficient) code:
Bar<Foo> a; Bar<Foo> b(a);
This is not equivalent. a is an lvalue here. In the above code, Bar<Foo>() is an rvalue. It should use the move constructor, not the copy constructor.
Le 25/08/14 23:23, Peter Dimov a écrit :
Ion Gaztañaga wrote:
Maybe you should design your code in C++11 and port it to Boost.Move, you might get better compiler help.
His code compiles in C++11, as is. I guess that in C++11 Bar<Foo>() is an rvalue that better matches Bar(Bar&&) than Bar(const &Bar), but with C++98 Bar<Foo>() is an rvalue that matches better const &Bar than Bar(rv<Bar>) as the last need a conversion.
If this copy constructor is instantiated, and in many compilers
Bar<Foo> b(( Bar<Foo>() ));
requires the copy constructor, then you get a compilation error. Just try this equivalent (but more inefficient) code:
Bar<Foo> a; Bar<Foo> b(a);
This is not equivalent. a is an lvalue here. In the above code, Bar<Foo>() is an rvalue. It should use the move constructor, not the copy constructor.
Hi, I suspect that Bar copy constructor should be available only if T is copy constructible. Unfortunately I don't know a way to achieve this. Best, Vicente
Vicente J. Botet Escriba wrote:
I suspect that Bar copy constructor should be available only if T is copy constructible. Unfortunately I don't know a way to achieve this.
Boost.Move's macros add a nested typedef, whose presence may in principle be exploited for SFINAE-ing out the copy constructor when T is move-only.
El 25/08/2014 23:23, Peter Dimov escribió:
Ion Gaztañaga wrote:
Maybe you should design your code in C++11 and port it to Boost.Move, you might get better compiler help.
His code compiles in C++11, as is.
If this copy constructor is instantiated, and in many compilers
Bar<Foo> b(( Bar<Foo>() ));
requires the copy constructor, then you get a compilation error. Just try this equivalent (but more inefficient) code:
Bar<Foo> a; Bar<Foo> b(a);
This is not equivalent. a is an lvalue here. In the above code, Bar<Foo>() is an rvalue. It should use the move constructor, not the copy constructor.
Yes, but he is compiling in C++03, that's why he's getting: error: passing ‘const Foo’ as ‘this’ argument of ‘Foo::operator boost::rv<Foo>&()’ discards qualifiers [-fpermissive] In C++03 both will require the copy constructor (I think). Best, Ion
On Mon, 25 Aug 2014 15:23:23 -0700, Ion Gaztañaga <igaztanaga@gmail.com> wrote:
El 25/08/2014 23:23, Peter Dimov escribió:
Ion Gaztañaga wrote:
Maybe you should design your code in C++11 and port it to Boost.Move, you might get better compiler help.
His code compiles in C++11, as is.
If this copy constructor is instantiated, and in many compilers
Bar<Foo> b(( Bar<Foo>() ));
requires the copy constructor, then you get a compilation error. Just try this equivalent (but more inefficient) code:
Bar<Foo> a; Bar<Foo> b(a);
This is not equivalent. a is an lvalue here. In the above code, Bar<Foo>() is an rvalue. It should use the move constructor, not the copy constructor.
Yes, but he is compiling in C++03, that's why he's getting:
error: passing ‘const Foo’ as ‘this’ argument of ‘Foo::operator boost::rv<Foo>&()’ discards qualifiers [-fpermissive]
In C++03 both will require the copy constructor (I think).
Then why is it that in C++03 with g++ 4.8.2 the code sample compiles if the copy ctor for Bar is not explicitly defined? My understanding is that in that case the copy ctor is *not* deleted, because the following compiles: // (3) Bar<int> b2(( Bar<int>() )); (Note, the definition of Foo's ctor has been changed to just take one arg and Bar's ctor has been updated accordingly.)
Mostafa wrote:
Then why is it that in C++03 with g++ 4.8.2 the code sample compiles if the copy ctor for Bar is not explicitly defined?
C++03 12.8/5: The implicitly-declared copy constructor for a class X will have the form X::X(const X&) if — each direct or virtual base class B of X has a copy constructor whose first parameter is of type const B& or const volatile B&, and — for all the nonstatic data members of X that are of a class type M (or array thereof), each such class type has a copy constructor whose first parameter is of type const M& or const volatile M&.107) Otherwise, the implicitly declared copy constructor will have the form X::X(X&) My guess is that Boost.Move declares Foo's copy constructor to take Foo&. This causes Bar's implicit copy constructor to take Bar&, which doesn't match rvalues.
On Mon, 25 Aug 2014 16:18:28 -0700, Peter Dimov <lists@pdimov.com> wrote:
Mostafa wrote:
Then why is it that in C++03 with g++ 4.8.2 the code sample compiles if the copy ctor for Bar is not explicitly defined?
C++03 12.8/5:
The implicitly-declared copy constructor for a class X will have the form X::X(const X&) if — each direct or virtual base class B of X has a copy constructor whose first parameter is of type const B& or const volatile B&, and — for all the nonstatic data members of X that are of a class type M (or array thereof), each such class type has a copy constructor whose first parameter is of type const M& or const volatile M&.107) Otherwise, the implicitly declared copy constructor will have the form X::X(X&)
My guess is that Boost.Move declares Foo's copy constructor to take Foo&. This causes Bar's implicit copy constructor to take Bar&, which doesn't match rvalues.
(Filling in the details.) Ah, so in that case Bar's move ctor is selected: Bar(BOOST_RV_REF(Bar) rhs) : f(::boost::move(rhs.f)) {} because "Bar<Foo>()" is implicitly convertible to BOOST_RV_REF(Bar). Thanks, Mostafa
El 26/08/2014 1:18, Peter Dimov escribió:
Mostafa wrote:
Then why is it that in C++03 with g++ 4.8.2 the code sample compiles if the copy ctor for Bar is not explicitly defined?
C++03 12.8/5:
The implicitly-declared copy constructor for a class X will have the form X::X(const X&) if — each direct or virtual base class B of X has a copy constructor whose first parameter is of type const B& or const volatile B&, and — for all the nonstatic data members of X that are of a class type M (or array thereof), each such class type has a copy constructor whose first parameter is of type const M& or const volatile M&.107) Otherwise, the implicitly declared copy constructor will have the form X::X(X&)
My guess is that Boost.Move declares Foo's copy constructor to take Foo&. This causes Bar's implicit copy constructor to take Bar&, which doesn't match rvalues.
Right. From boost/move/core.hpp: #define BOOST_MOVABLE_BUT_NOT_COPYABLE(TYPE)\ BOOST_MOVE_IMPL_NO_COPY_CTOR_OR_ASSIGN(TYPE)\ public:\ operator ::boost::rv<TYPE>&() \ { return *static_cast< ::boost::rv<TYPE>* >(this); }\ operator const ::boost::rv<TYPE>&() const \ { return *static_cast<const ::boost::rv<TYPE>* >(this); }\ private:\ // and #if defined(BOOST_NO_CXX11_DELETED_FUNCTIONS) #define BOOST_MOVE_IMPL_NO_COPY_CTOR_OR_ASSIGN(TYPE) \ private:\ TYPE(TYPE &);\ TYPE& operator=(TYPE &);\ public:\ typedef int boost_move_no_copy_constructor_or_assign; \ private:\ // #else #define BOOST_MOVE_IMPL_NO_COPY_CTOR_OR_ASSIGN(TYPE) \ public:\ TYPE(TYPE const &) = delete;\ TYPE& operator=(TYPE const &) = delete;\ public:\ typedef int boost_move_no_copy_constructor_or_assign; \ private:\ // #endif //BOOST_NO_CXX11_DELETED_FUNCTIONS Best, Ion
participants (4)
-
Ion Gaztañaga -
Mostafa -
Peter Dimov -
Vicente J. Botet Escriba