Skip to content

Detect std::pair non-copyability #965

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

Merged
merged 1 commit into from
Jul 29, 2017
Merged

Conversation

jagerman
Copy link
Member

Pre-C++17, std::pair can technically have an copy constructor even
though it can't actually be invoked without a compilation failure (due
to the underlying types being non-copyable). Most stls, including
libc++ since ~3.4, use the C++17 behaviour of not exposing an uncallable
copy constructor, but FreeBSD deliberately broke their libc++ to
preserve the nonsensical behaviour
(https://svnweb.freebsd.org/base?view=revision&revision=261801).

This updates pybind's internal is_copy_constructible to also detect
the std::pair case under pre-C++17.

This also updates a few places that were using
std::is_copy_constructible to use the internal version instead.

Fixes #964

@jagerman
Copy link
Member Author

jagerman commented Jul 27, 2017

I think this might not be enough. Investigating. Fixed and updated.

Pre-C++17, std::pair can technically have an copy constructor even
though it can't actually be invoked without a compilation failure (due
to the underlying types being non-copyable).  Most stls, including
libc++ since ~3.4, use the C++17 behaviour of not exposing an uncallable
copy constructor, but FreeBSD deliberately broke their libc++ to
preserve the nonsensical behaviour
(https://svnweb.freebsd.org/base?view=revision&revision=261801).

This updates pybind's internal `is_copy_constructible` to also detect
the std::pair case under pre-C++17.

This also everything (except for a couple cases in the internal version)
to use the internal `is_copy_constructible` rather than
`std::is_copy_constructible`.
@rleigh-codelibre
Copy link

Thanks for looking at this so quickly!

Test results:

Standard FreeBSD 11.0 FreeBSD 11.1
14 OK OK
17 FAIL OK

It now builds on FreeBSD 11.0 with C++14. However, building with -DCMAKE_CXX_STANDARD=17 fails with 11.0/clang++ 3.8.0 in the same manner as before: https://gist.github.com/rleigh-codelibre/387d25bf9da258fd80135e1a64e11335

Clang/LLVM 4.0.0 drops this option entirely, so it's now properly standards conformant from this point onward on FreeBSD.

@jagerman
Copy link
Member Author

clang 3.8/C++17 is definitely unsupported (the travis-ci builds test against g++ 7 and clang/libc++ 4.0 for C++17 support). If you really need it you can try compiling with -D_LIBCPP_TRIVIAL_PAIR_COPY_CTOR=1, but realistically getting decent C++17 compiler and stl support requires a newer compiler.

@jagerman
Copy link
Member Author

Also note that if you never try creating a std::pair with a non-copy-constructible type you won't actually hit the error; the test suite has various tests designed to hit various edge cases.

@rleigh-codelibre
Copy link

OK, thanks. I'll have to look at upgrading to 4.0.0 soon, so I can use C++17 without the tests failing.

@jagerman jagerman merged commit 7937260 into pybind:master Jul 29, 2017
@dean0x7d dean0x7d modified the milestone: v2.2 Aug 13, 2017
@jagerman jagerman deleted the pair-copyability branch August 14, 2017 20:25
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.

3 participants