rjhogan / Adept-2

Combined array and automatic differentiation library in C++
http://www.met.reading.ac.uk/clouds/adept/
Apache License 2.0
163 stars 29 forks source link

Array& operator=(Array&& rhs) fails to compile. #6

Open JackCibc opened 6 years ago

JackCibc commented 6 years ago

0001-Initial-move-assign-fixes.txt

The following test case (a call to Array(.) move assignment) fails to compile on Visual Studio 15.5.3 and Clang, with error message:

'1>aad\adept-2\adept-2.0.4\include\adept\array.h(402): error C2660: 'adept::internal::GradientIndex::swap': function does not take 2 arguments' '1>aad\adept-2\adept-2.0.4\include\adept\array.h(385): note: while compiling class template member function 'adept::Array<1,adept::Real,false> &adept::Array<1,adept::Real,false>::operator =(adept::Array<1,adept::Real,false> &&)''

I've attached a simple fix, but it may not be the best way to resolve this issue.

Regards,

John.

include

include <adept/array_shortcuts.h>

include < iostream >

using adept::adouble; using adept::Vector;

Vector generateVector(void) { Vector x = { 3.0 }; return x; }

void testMoveAssign(void) {

ifdef ADEPT_MOVE_SEMANTICS

std::cout << "\n Move on" << std::endl;

else

std::cout << "\n Move off" << std::endl;

endif

Vector v;
v = generateVector();    // This fails to compile.

}

rjhogan commented 6 years ago

Hi,

I can't reproduce this with Clang 3.5 on Linux, and my correspondent who uses Adept on Windows with VS17.x reported some other issues with move semantics (which we fixed) but not this. So perhaps it's specific to VS15.x?  I'm willing to make the changes you suggest to Array.h if they don't break any other compiler (I can't see that they would).

But can I ask if your change to ExpressionSize.h is necessary? "swap" is always implemented as a friend in order that it can be found from outside the namespace by argument-dependent lookup; implementing it as static will not achieve this.

Thanks,

Robin.

On 16/01/18 10:05, JackCibc wrote:

0001-Initial-move-assign-fixes.txt https://github.com/rjhogan/Adept-2/files/1634500/0001-Initial-move-assign-fixes.txt

The following test case (a call to Array(.) move assignment) fails to compile on Visual Studio 15.5.3 and Clang, with error message:

'1>aad\adept-2\adept-2.0.4\include\adept\array.h(402): error C2660: 'adept::internal::GradientIndex::swap': function does not take 2 arguments' '1>aad\adept-2\adept-2.0.4\include\adept\array.h(385): note: while compiling class template member function 'adept::Array<1,adept::Real,false> &adept::Array<1,adept::Real,false>::operator =(adept::Array<1,adept::Real,false> &&)''

I've attached a simple fix, but it may not be the best way to resolve this issue.

Regards,

John.

include

include <adept/array_shortcuts.h>

include < iostream >

using adept::adouble; using adept::Vector;

Vector generateVector(void) { Vector x = { 3.0 }; return x; }

void testMoveAssign(void) {

ifdef ADEPT_MOVE_SEMANTICS

std::cout << "\n Move on" << std::endl;

else

std::cout << "\n Move off" << std::endl;

endif

Vector v; v = generateVector(); // This fails to compile. }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rjhogan/Adept-2/issues/6, or mute the thread https://github.com/notifications/unsubscribe-auth/AMWiWTKz7OgIirnGt2TB3b6BtZoq2kJzks5tLHR1gaJpZM4RfhYl.

-- Professor Robin J. Hogan Department of Meteorology University of Reading PO Box 243 READING RG6 6BB, UK

http:://www.met.rdg.ac.uk/~swrhgnrj/

JackCibc commented 6 years ago

Thanks. This was Visual Studio 2017 - confusingly the version number is 15.5.x. I've now also tested with 15.5.6 and Adept-2\master from today and I get the same result. The Clang is the one that ships with Visual Studio, "Clang parser and MS code-gen", so may also have some Microsoft specific changes unfortunately.

I take your point on swap(.) - I've moved it into the class by making it static. VS isn't finding it otherwise, and I didn't find a good way to make it a free function.

Cheers,

John.

manuelnp commented 6 years ago

Hi Jack,

I was able to build you example in my VS2012 successfully (removing the initialization list being that c++11 subset in VS2012 does not include them) so it seems more like a VS2017 issue. I will perform further test and let you know.

Regards.

JackCibc commented 6 years ago

Could you check if the move assignment is being compiled in? From the docs it looks like _MSVC_LANG wasn't defined until VS 2015, so you may not be getting move semantics at all. I can't check right now I'm afraid.

An easy check would be to do

ifdef ADEPT_MOVE_SEMANTICS

pragma message( "\n Move on")

else

pragma message( "\n Move off")

endif

in the test program.