Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create an actual copy during director pass-by-value. #434

Closed
wants to merge 1 commit into from

Conversation

LindleyF
Copy link
Contributor

Handle arguments passed by value into a director class the same way as arguments outputted by value: Make a copy of them that Java can own.

It would be nice if the new object could be move-initialized, but in the interest of compatibility I have not attempted this. Is there a policy on how move semantics can be exploited in SWIG without breaking compatibility?

Objects passed by value are only valid for the duration of the call, so the previous approach made sense from a C++ perspective; however, in C++ you could save the argument by copying or moving it. In Java it's more natural to save a reference to the object instead. This approach allows this to work.

Perhaps ironically, pass-by-reference still results in an object that Java cannot safely safe a reference to, because objects passed by reference might not be copy-constructable.

@LindleyF
Copy link
Contributor Author

I'm confused. All the tests appear to have passed, so why does Travis think there's a problem?

@LindleyF
Copy link
Contributor Author

Ping?

@wsfulton
Copy link
Member

wsfulton commented Sep 9, 2015

Looks like Travis had a clock problem:

checking whether build environment is sane... configure: error: newly created file is older than distributed files!

Check your system clock

It seems too old to restart, so I think we can take it that there is no problem.

With regard to move semantics, SWIG needs to generate c++98 compatible code, so you can't use std::move. However, if SWIG generates code where a temporary is passed, a C++11 compiler will call the move constructor and a C++98 compiler won't, so it is sometimes possible to generate C++98 code that will take advantage of move semantics. Outside of directors, you may be interested in the 'optimal' typemap attribute to help RVO, see http://swig.org/Doc3.0/Typemaps.html#Typemaps_optimal

Directors are a C++ only SWIG feature so please remove the C code and the #ifdef __cplusplus addition to the patch.

@wsfulton
Copy link
Member

I gave this some thought today. The proposed change of making a C++ copy is actually the most correct approach. It'll make performance worse though and there is a small risk it will break existing code (where there is no true copy constructor but there is a move constructor, eg auto_ptr/unique_ptr type smart pointers). Nevertheless, I think we ought to consider this change for all languages as the problem is no different to wrapping an api that returns one of these types.

Regarding the Java patch, you need to re-instate the $input = 0; code as otherwise some of the bytes in the $input variable may not be initialized, depending on pointer sizes.

Regarding move semantics, as SWIG targets C++ 98, users would have to override this typemap and use std::move.

@LindleyF
Copy link
Contributor Author

I'll get back to this soon. Regarding the movable type issue issue, unfortunately we can't use the 'optimal' trick since the parameters can't be turned into xvalues implicitly. I'm not thrilled at the idea of requiring move-only types to write their own typemaps either; that seems like it might have been okay a few years ago, but unique_ptrs in APIs are getting pretty common these days.

What I'd like to do is define a SWIG_MOVE macro which expands to std::move(T) in C++11 and to (T) in C++98. I'm not certain there's a perfect way to condition it, though. I found this on stackoverflow:

#if cplusplus >= 201103L || defined(__GXX_EXPERIMENTAL_CXX0X) ||
defined(_MSC_VER) && _MSC_VER >= 1600
#define HAS_MOVE_SEMANTICS 1
#elif defined(__has_feature)
#if __has_feature(cxx_rvalue_references)
#define HAS_MOVE_SEMANTICS 1
#else
#define HAS_MOVE_SEMANTICS 0
#endif
#else
#define HAS_MOVE_SEMANTICS 0
#endif

But I haven't validated it yet. Do you have any objection to incorporating this sort of approach into this pull request?

@wsfulton
Copy link
Member

The same issue was reported against Python a while back and I've just replied with the required Python typemap, see https://sourceforge.net/p/swig/mailman/message/35086539/.

@LindleyF I suggest a separate pull request for providing move semantics support. I'd rather have this concept across all languages though, but a prototype against Java to begin with should work.

@wsfulton wsfulton closed this in 98a31ff May 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants