[move] problem with msvc and BOOST_MOVABLE_BUT_NOT_COPYABLE
I get an linker error (private: __thiscall X::X(class X &) is unresolved symbol) with msvc-9.0 on XP. AFAIL (docu) implementing the copy-ctor isn't required and passing values from static memeber functions are allowed. What's wrong? Oliver #include "stdafx.h" #include <boost/move/move.hpp> class X { private: BOOST_MOVABLE_BUT_NOT_COPYABLE( X); public: X(); static X create(); X( BOOST_RV_REF( X) other); X & operator=( BOOST_RV_REF( X) other); }; X::X() {} X X::create() { X x; return x; // passing value form static memeber-fn } X::X( BOOST_RV_REF( X) other) {} X & X::operator=( BOOST_RV_REF( X) other) { return * this; } int _tmain(int argc, _TCHAR* argv[]) { X x( X::create() ); return 0; } -- GRATIS für alle GMX-Mitglieder: Die maxdome Movie-FLAT! Jetzt freischalten unter http://portal.gmx.net/de/go/maxdome01
Oliver Kowalke wrote:
I get an linker error (private: __thiscall X::X(class X &) is unresolved symbol) with msvc-9.0 on XP. AFAIL (docu) implementing the copy-ctor isn't required and passing values from static memeber functions are allowed.
What's wrong? [snip] X X::create() { X x; return x; // passing value form static memeber-fn }
The statement "return x" must make some constructor call. As x is not a temporary, the compiler must call the copy constructor. It will probably work if you write "return boost::move(x)" or "return X()". Regards, Thomas
Am 27.05.2010 21:28, schrieb Thomas Klimpel:
The statement "return x" must make some constructor call. As x is not a temporary, the compiler must call the copy constructor. It will probably work if you write "return boost::move(x)" or "return X()".
thx - maybe the docu should be more clear in this point
Oliver Kowalke wrote:
I get an linker error (private: __thiscall X::X(class X &) is unresolved symbol) with msvc-9.0 on XP. AFAIL (docu) implementing the copy-ctor isn't required and passing values from static memeber functions are allowed.
What's wrong?
Oliver
#include "stdafx.h"
#include <boost/move/move.hpp>
class X { private: BOOST_MOVABLE_BUT_NOT_COPYABLE( X); public: X(); static X create(); X( BOOST_RV_REF( X) other); X & operator=( BOOST_RV_REF( X) other); };
X::X() {}
X X::create() { X x; return x; // passing value form static memeber-fn }
X::X( BOOST_RV_REF( X) other) {}
X & X::operator=( BOOST_RV_REF( X) other) { return * this; }
int _tmain(int argc, _TCHAR* argv[]) { X x( X::create() ); return 0; }
Could BOOST_MOVABLE_BUT_NOT_COPYABLE be changed into CRTP base class? It would prevent such errors at compile time. Also, it is quite similar to boost::noncopyable.
Ilya Sokolov wrote:
Could BOOST_MOVABLE_BUT_NOT_COPYABLE be changed into CRTP base class? It would prevent such errors at compile time. Also, it is quite similar to boost::noncopyable.
It seems my answer to Oliver wasn't clear enough. This is not a problem of Boost.Move, but a simple programmer error in "X X::create()" that would have a significant performance penalty even if the compiler/linker would accept the code. However, a CRTP class might be used to convert between the optimized mode with unhappy assignment operator signature and the non-optimized mode. Regards, Thomas
Thomas Klimpel wrote:
Ilya Sokolov wrote:
Could BOOST_MOVABLE_BUT_NOT_COPYABLE be changed into CRTP base class? It would prevent such errors at compile time. Also, it is quite similar to boost::noncopyable.
It seems my answer to Oliver wasn't clear enough. This is not a problem of Boost.Move,
Yes, it is (arguably). The problem is that error is reported too late.
but a simple programmer error in "X X::create()" that would have a significant performance penalty even if the compiler/linker would accept the code.
There is no implementation of copy ctor in the original example. How compiler/linker could accept the code? What "performance penalty" you are talking about?
However, a CRTP class might be used to convert between the optimized mode with unhappy assignment operator signature and the non-optimized mode.
Probably, but that is not important for me. I like the following benefits: - one macro less - uniformity between boost::movable_only and boost::noncopyable
Ilya Sokolov wrote:
Thomas Klimpel wrote:
It seems my answer to Oliver wasn't clear enough. This is not a problem of Boost.Move,
Yes, it is (arguably). The problem is that error is reported too late.
From my point of view, there is not much difference between link-time and compile-time. An error at run-time would be too late.
but a simple programmer error in "X X::create()" that would have a significant performance penalty even if the compiler/linker would accept the code.
There is no implementation of copy ctor in the original example. How compiler/linker could accept the code? What "performance penalty" you are talking about?
I'm talking about the BOOST_COPYABLE_AND_MOVABLE case. Most compilers won't do RVO for code written like "X X::create()".
Probably, but that is not important for me. I like the following benefits: - one macro less - uniformity between boost::movable_only and boost::noncopyable
You're probably right that a macro "BOOST_MOVABLE" just containing the conversion operators would be enough for the non-optimized mode. The BOOST_MOVABLE_BUT_NOT_COPYABLE case would derive privately from boost::movable_only and implement move constructor and move assignment operator, the BOOST_COPYABLE_AND_MOVABLE would just implement the correct move constructor and assignment operator. Regards, Thomas
At Fri, 28 May 2010 18:29:13 +0200, Thomas Klimpel wrote:
but a simple programmer error in "X X::create()" that would have a significant performance penalty even if the compiler/linker would accept the code.
There is no implementation of copy ctor in the original example. How compiler/linker could accept the code? What "performance penalty" you are talking about?
I'm talking about the BOOST_COPYABLE_AND_MOVABLE case. Most compilers won't do RVO for code written like "X X::create()".
You are claiming that some aspect of that signature causes RVO not to happen? What aspect? -- Dave Abrahams Meet me at BoostCon: http://www.boostcon.com BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
I'm talking about the BOOST_COPYABLE_AND_MOVABLE case. Most compilers won't do RVO for code written like "X X::create()".
You are claiming that some aspect of that signature causes RVO not to happen? What aspect?
I was talking about the signature, but about the way "X X::create()" was written: X X::create() { X x; return x; // passing value form static memeber-fn } The expectation that the statement "return x;" will benefit from RVO might not be justified here. Writing "return boost::move(x);" might actually be a good idea. Regards, Thomas
On 28/05/2010 19:32, Thomas Klimpel wrote:
The expectation that the statement "return x;" will benefit from RVO might not be justified here. Writing "return boost::move(x);" might actually be a good idea.
GCC 4.4.1 with -std=gnu++0x behaves similarly to boost.move emulation: volatile int cond = 0; class X { X(const X&); X& operator=(const X&); public: X(){} X(X&&){} X& operator=(X&&){} static X create() { X x, x2; return cond ? x : x2; // passing value form static member-fn } }; int main() { X x(X::create()); return 0; } $ gcc test.cpp -std=gnu++0x C:\Users\IGAZTA~1\AppData\Local\Temp\ccUoS3Ag.o:test.cpp:(.text$_ZN1X6createEv[X::create()]+0x46): undefined reference to `X::X(X const&)' collect2: ld returned 1 exit status Same with MSVC-10.0: 1>------ Build started: Project: test, Configuration: Debug Win32 ------ 1> test.cpp 1>test.obj : error LNK2001: unresolved external symbol "private: __thiscall X::X(class X const &)" (??0X@@AAE@ABV0@@Z) 1>c:\users\igaztanaga\documents\visual studio 2010\Projects\test\Debug\test.exe : fatal error LNK1120: 1 unresolved externals But I would expect a compile-time error. All fine if we change the return statement to: return cond ? static_cast<X&&>(x) : static_cast<X&&>(x2); Best, Ion
On 28/05/2010 21:27, Ilya Sokolov wrote:
X(const X&) = delete; X& operator=(const X&) = delete;
Ok, but we are talking about rvalue references and asking if Boost.Move provokes some late error report. It's a shame we can't provoke a compilation error always (even with NVRO, I thought the compiler should check for the copy constructor, but I'm not an expert), but that's the best we can do. Best, Ion
Ion Gaztañaga wrote:
On 28/05/2010 21:27, Ilya Sokolov wrote:
X(const X&) = delete; X& operator=(const X&) = delete;
Ok, but we are talking about rvalue references and asking if Boost.Move provokes some late error report. It's a shame we can't provoke a compilation error always (even with NVRO, I thought the compiler should check for the copy constructor, but I'm not an expert),
I'm not an expert either, but 12.2/1 of 14882:2003 clearly says so. AFAIR, msvc doesn't check.
but that's the best we can do.
template<class TYPE> class movable_but_not_copyable { TYPE(TYPE &); TYPE& operator=(TYPE &); public: operator ::BOOST_MOVE_NAMESPACE::rv<TYPE>&(); operator const ::BOOST_MOVE_NAMESPACE::rv<TYPE>&() const; }; // from the original example class X : public movable_but_not_copyable { /*...*/ }; X X::create() { X x; return x; // compile-time error What do you think? Or even class noncopyable { // note: non-const ref noncopyable(noncopyable&); noncopyable& operator=(noncopyable&); }; template<class TYPE> struct movable { operator ::BOOST_MOVE_NAMESPACE::rv<TYPE>&(); operator const ::BOOST_MOVE_NAMESPACE::rv<TYPE>&() const; };
On 28/05/2010 23:30, Ilya Sokolov wrote:
Ion Gaztañaga wrote: What do you think?
It's an option, I just was trying to say that we get a similar guarantee that a simple rvalue reference based implementation could offer. If we should add more guarantees because the compiler diagnostics are not good enough, I'm open to suggestions. Adding a base class is not something I like because in some C++03 (Msvc I think, empty base classes are not optimized with multiple inheritance), although maybe the benefits outweigh the problems. Thanks for your suggestions, Ion
Ion Gaztañaga wrote:
Adding a base class is not something I like because in some C++03 (Msvc I think, empty base classes are not optimized with multiple inheritance),
IIRC, compilers don't do SBO if the same class is used as a base multiple times. However, movable<> will be instantiated with different types most of the time.
On 29/05/2010 0:24, Ilya Sokolov wrote:
Ion Gaztañaga wrote:
Adding a base class is not something I like because in some C++03 (Msvc I think, empty base classes are not optimized with multiple inheritance),
IIRC, compilers don't do SBO if the same class is used as a base multiple times. However, movable<> will be instantiated with different types most of the time.
Taken from: http://social.msdn.microsoft.com/Forums/en-US/vcgeneral/thread/6502c2d3-7c06... #include <cstdio> class E1 { public: void ReportE1() { std::printf("In E1, this is %p\n", this); } }; class E2 { public: void ReportE2() { std::printf("In E2, this is %p\n", this); } }; class E3 { public: void ReportE3() { std::printf("In E3, this is %p\n", this); } }; class D: public E1, public E2, public E3 { }; int main() { D d; d.ReportE1(); d.ReportE2(); d.ReportE3(); } Reports the same this pointer values if compiled using G++; However, when I compile it using visual C++ 2010 express, the values reported increment by 1, say 10000,10001,10002. Best, Ion
At Fri, 28 May 2010 22:31:11 +0200, Ion Gaztañaga wrote:
On 28/05/2010 21:27, Ilya Sokolov wrote:
X(const X&) = delete; X& operator=(const X&) = delete;
Ok, but we are talking about rvalue references and asking if Boost.Move provokes some late error report. It's a shame we can't provoke a compilation error always (even with NVRO, I thought the compiler should check for the copy constructor,
Yes, a conforming compiler checks for it and issues a diagnostic if it isn't accessible, whether or not it's actually used.
but I'm not an expert), but that's the best we can do.
-- Dave Abrahams Meet me at BoostCon: http://www.boostcon.com BoostPro Computing http://www.boostpro.com
Am 28.05.2010 20:13, schrieb Ion Gaztañaga:
GCC 4.4.1 with -std=gnu++0x behaves similarly to boost.move emulation: <snip> $ gcc test.cpp -std=gnu++0x C:\Users\IGAZTA~1\AppData\Local\Temp\ccUoS3Ag.o:test.cpp:(.text$_ZN1X6createEv[X::create()]+0x46): undefined reference to `X::X(X const&)' collect2: ld returned 1 exit status
With gcc-4.4.3 I don't get the linker error (using my example with boost.move) and msvc-9.0 for tests only.
Oliver Kowalke wrote:
With gcc-4.4.3 I don't get the linker error (using my example with boost.move) and msvc-9.0 for tests only.
This indicates to me that Dave is right when he say that NRVO is already the norm and not the exception, even in debug mode.
At Fri, 28 May 2010 19:32:45 +0200, Thomas Klimpel wrote:
David Abrahams wrote:
I'm talking about the BOOST_COPYABLE_AND_MOVABLE case. Most compilers won't do RVO for code written like "X X::create()".
You are claiming that some aspect of that signature causes RVO not to happen? What aspect?
I was talking about the signature, but about the way "X X::create()" was written:
X X::create() { X x; return x; // passing value form static memeber-fn }
A static member function is just a function to the compiler. AFAIK, the only compiler in recent memory that doesn't have NRVO (yet) is Clang. And they're working on it. What compiler are you talking about?
The expectation that the statement "return x;" will benefit from RVO might not be justified here. Writing "return boost::move(x);" might actually be a good idea.
There are other scenarios where NRVO won't go into effect and return boost::move(x) is warranted, X create(bool which) { X a("foo"); X b("bar"); return which ? a : b; } but I don't see anything like that in this case. -- Dave Abrahams Meet me at BoostCon: http://www.boostcon.com BoostPro Computing http://www.boostpro.com
David Abrahams wrote:
You are claiming that some aspect of that signature causes RVO not to happen? What aspect?
I understand you want to comment on what could be FUD, but why do you ask about the "signature" then?
A static member function is just a function to the compiler. AFAIK, the only compiler in recent memory that doesn't have NRVO (yet) is Clang. And they're working on it. What compiler are you talking about?
Same here, I appreciate it that you try to clarify my misconceptions, but I was talking about RVO here, not NRVO. Please also understand my position that everybody is allowed to make mistakes, but he shouldn't blame Boost.Move or its documentation for it. The linker had complained about a missing symbol, and this means that it had indeed generated the corresponding constructor call. This might be related to compiling in debug mode with msvc-9.0. I don't know whether gcc-4.5 or msvc-10.0 implement NRVO even in debug mode. However, this should be easy to find out if it's important for this discussion. Regards, Thomas
At Fri, 28 May 2010 21:03:24 +0200, Thomas Klimpel wrote:
David Abrahams wrote:
You are claiming that some aspect of that signature causes RVO not to happen? What aspect?
I understand you want to comment on what could be FUD, but why do you ask about the "signature" then?
Because that's all you showed in the first message? There was no body.
A static member function is just a function to the compiler. AFAIK, the only compiler in recent memory that doesn't have NRVO (yet) is Clang. And they're working on it. What compiler are you talking about?
Same here, I appreciate it that you try to clarify my misconceptions, but I was talking about RVO here, not NRVO.
In the function you wrote in your previous message: X X::create() { X x; return x; // passing value form static memeber-fn } x is named. Its name is “x” :-). Optimizing away the copy requires the Named Return Value Optimization (NRVO), not the Unnamed Return Value Optimization (URVO). “RVO” by itself refers to both of them.
Please also understand my position that everybody is allowed to make mistakes, but he shouldn't blame Boost.Move or its documentation for it. The linker had complained about a missing symbol, and this means that it had indeed generated the corresponding constructor call. This might be related to compiling in debug mode with msvc-9.0. I don't know whether gcc-4.5 or msvc-10.0 implement NRVO even in debug mode. However, this should be easy to find out if it's important for this discussion.
Sorry, I haven't followed the rest of the discussion so the above doesn't mean much to me. -- Dave Abrahams Meet me at BoostCon: http://www.boostcon.com BoostPro Computing http://www.boostpro.com
Am 28.05.2010 21:03, schrieb Thomas Klimpel:
Please also understand my position that everybody is allowed to make mistakes, but he shouldn't blame Boost.Move or its documentation for it. Did I blame boost.move?
Oliver Kowalke wrote:
Thomas Klimpel wrote:
Please also understand my position that everybody is allowed to make mistakes, but he shouldn't blame Boost.Move or its documentation for it. Did I blame boost.move?
Nobody blamed boost.move directly, but there were many constructive suggestions how boost.move could be improved in the face of this simple mistake. I got the impression that the discussion blamed BOOST_MOVABLE_BUT_NOT_COPYABLE, so I pointed out that BOOST_COPYABLE_AND_MOVABLE runs into performance problem for the cases where BOOST_MOVABLE_BUT_NOT_COPYABLE runs into link problems, and you can't blame boost.move for this.
maybe the docu should be more clear in this point
My initial though was that the docu should provide references to existing external documentation where such things are explained in sufficient detail. After this discussion, I ask myself whether the documentation should say something about RVO (or call is URVO if you want) versus NRVO. The idea by Ilya Sokolov to modify the signature of the private copy constructor and assignment operator in boost::noncopyable to a non-const reference actually seems to make perfect sense. After all, a class that is movable can still be noncopyable, so why not use boost::noncopyable to incidate that it is noncopyable? Well, it will only work after applying the suggested "fix" to boost::noncopyable. Regards, Thomas
On 29/05/2010 1:29, Thomas Klimpel wrote:
The idea by Ilya Sokolov to modify the signature of the private copy constructor and assignment operator in boost::noncopyable to a non-const reference actually seems to make perfect sense.
It's a good option, but I'm still reluctant. Adding a base class in boost namespace has also implications with ADL and it's unnecessary for compilers with deleted definitions (= delete). I need a bit more time to see pros/cons. Best, Ion
On 5/28/2010 11:45 PM, Ion Gaztañaga wrote:
On 29/05/2010 1:29, Thomas Klimpel wrote:
The idea by Ilya Sokolov to modify the signature of the private copy constructor and assignment operator in boost::noncopyable to a non-const reference actually seems to make perfect sense.
It's a good option, but I'm still reluctant. Adding a base class in boost namespace has also implications with ADL and it's unnecessary for compilers with deleted definitions (= delete). I need a bit more time to see pros/cons.
Best,
Ion
I, too, am (currently) against adding such a base class, for the above reasons and for the MSVC EBO reason Ion gave earlier. - Jeff
Ion Gaztañaga wrote:
It's a good option, but I'm still reluctant. Adding a base class in boost namespace has also implications with ADL and it's unnecessary for compilers with deleted definitions (= delete). I need a bit more time to see pros/cons.
I think boost::noncopyable is a typedef for boost::noncopyable_::noncopyable. But you are right, it leads to lookup in the namespace boost. But I wonder why it is not implemented as a typedef for boost_noncopyable::noncopyable, as this would seem to avoid the ADL problems. Jeffrey Lee Hellrung wrote:
I, too, am (currently) against adding such a base class, for the above reasons and for the MSVC EBO reason Ion gave earlier.
I interpreted the proposal in the way that Ilya Sokolov was thinking loudly about modifying boost::noncopyable to suit the need of boost.move. But the problems with unreliable EBO are really an issue for the other proposals of Ilya, as they also make the reinterpret_cast (when implemented in the base class) more dangerous. Regards, Thomas
On May 29, 2010, at 6:00 PM, Thomas Klimpel <Thomas.Klimpel@synopsys.com> wrote:
I think boost::noncopyable is a typedef for boost::noncopyable_::noncopyable. But you are right, it leads to lookup in the namespace boost.
IIUC if it does, that's a compiler bug. shocked-shocked-to-think-that-a-compiler-might-have-an-ADL-bug-ly y'rs, Dave
David Abrahams wrote:
Thomas Klimpel wrote:
I think boost::noncopyable is a typedef for boost::noncopyable_::noncopyable. But you are right, it leads to lookup in the namespace boost.
IIUC if it does, that's a compiler bug.
I guess you're right. I ran bjam with the attached tests for gcc-4.3.4, msvc-9.0express and msvc-10.0, and only msvc-9.0express failed the noncopyable_adl_test. All compilers successfully passed the noncopyable_adl_test_.
shocked-shocked-to-think-that-a-compiler-might-have-an-ADL-bug-ly y'rs,
This time I even tried to read about it in the documents references by wikipedia, but I wasn't able to figure out which would be the correct behavior. I slightly rewrote my test then, so that it really tests for the nested namespace and not for the "fixing ADL (revision 2)", but the modified test (noncopyable_adl_metal_test.cpp) gave the same results. Looks like implementing correct ADL for a C++ compiler is a daunting task. Regards, Thomas
Am 29.05.2010 01:29, schrieb Thomas Klimpel:
My initial though was that the docu should provide references to existing external documentation where such things are explained in sufficient detail. After this discussion, I ask myself whether the documentation should say something about RVO (or call is URVO if you want) versus NRVO.
I would say in section 'Movable but Non-Copyable Types' - after the example with the factory methods - 'In the above example, the underlying file handle is passed from object to object, as long as the source file_descriptor is an rvalue. At all times, there is still only one underlying file handle, and only one file_descriptor owns it at a time.' I would suggest to add a comment here about NRVO/URVO and the hint to return the object via boost.move().
participants (6)
-
David Abrahams -
Ilya Sokolov -
Ion Gaztañaga -
Jeffrey Lee Hellrung, Jr. -
Oliver Kowalke -
Thomas Klimpel