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

Set PYBIND11_CPP1{4,7} macros for MSVC #841

Merged
merged 2 commits into from
May 9, 2017

Conversation

jagerman
Copy link
Member

@jagerman jagerman commented May 8, 2017

We currently don't set PYBIND11_CPP14 or _CPP17 for MSVC because MSVC doesn't support the code in descr.h. Originally, that's all the _CPP14 flag guarded, but it now enables several other things as well (most of which are #ifdef CPP14 || MSVC). This flips that, setting CPP14 and CPP17 (the latter only when compiling under MSVC 2017 with /std:c++latest), then changing the descr.h code to require CPP14 && !MSVC.

This PR also enables the PYBIND11_CPP_STANDARD cmake variable under MSVC, defaulting it to /std:c++14, then uses it to add a /std:c++latest appveyor job which runs the optional and variant tests.

Fixes #833.

@wjakob
Copy link
Member

wjakob commented May 8, 2017

I'm curious -- what happens when using the constexpr descr with MSVC2017? When I tried this with MSVC2015 at some point, everything could be made to compile with minor tweaks -- however, when inspecting the resulting binary, it was impossible to find the function signature strings, suggesting that they were not precomputed at compile time.

@jagerman
Copy link
Member Author

jagerman commented May 8, 2017

It fails with: cast.h(1121): error C2131: expression did not evaluate to a constant and descr.h(91): note: failure was caused by non-constant arguments or reference to a non-constant symbol.

I didn't try investigating any further to try to fix it.

@RedSkotina
Copy link

ok. give me time for investigating then.

@jagerman
Copy link
Member Author

jagerman commented May 8, 2017

Another MSVC annoyance: it looks like

using std::enable_if_t;

doesn't work with MSVC 2015.

@jagerman
Copy link
Member Author

jagerman commented May 8, 2017

A #792 failure, but otherwise passes now (same code, no failure).

@RedSkotina
Copy link

pass all tests with and without -DPYBIND11_CPP_STANDARD=/std:c++latest on msvc 2017

@dean0x7d
Copy link
Member

dean0x7d commented May 8, 2017

LGTM

@RedSkotina
Copy link

can you then update documentation about -DPYBIND11_CPP_STANDARD=/std:c++latest ?

@jagerman
Copy link
Member Author

jagerman commented May 8, 2017

It's already documented, though it didn't mention the doesn't-do-anything-on-MSVC limitation (which this PR removes). I did reword things a bit, and added MSVC /std:c++... examples to the documentation.

@jagerman
Copy link
Member Author

jagerman commented May 8, 2017

It also changes the wording a bit to say that we explicitly target C++14 by default, rather than "the latest available," but mentions that it is subject to change to the latest available, which I think captures a little bit more clearly what we are doing.

The PYBIND11_CPP14 macro started out as a guard for the compile-time
path code in `descr.h`, but has since come to mean other things.  This
means that while the `descr.h` check has just checked the
`PYBIND11_CPP14` macro, various other places now check `PYBIND11_CPP14
|| _MSC_VER`.  This reverses that by now setting the CPP14 macro when
MSVC is trying to support C++14, but disabling the `descr.h` C++14 code
(which still fails under MSVC 2017).

The CPP17 macro also gets enabled when MSVC 2017 is compiling with
/std:c++latest (the default is /std:c++14), which enables
`std::optional` and `std::variant` support under MSVC.
Under MSVC we were ignoring PYBIND11_CPP_STANDARD and simply not
passing any standard (which makes MSVC default to its C++14 mode).

MSVC 2015u3 added the `/std:c++14` and `/std:c++latest` flags; the
latter, under MSVC 2017, enables some C++17 features (such as
`std::optional` and `std::variant`), so it is something we need to
start supporting under MSVC.

This makes the PYBIND11_CPP_STANDARD cmake variable work under MSVC,
defaulting it to /std:c++14 (matching the default -std=c++14 for
non-MSVC).

It also adds a new appveyor test running under MSVC 2017 with
/std:c++latest, which runs (and passes) the
`std::optional`/`std::variant` tests.

Also updated the documentation to clarify the c++ flags and add show
MSVC flag examples.
@jagerman jagerman merged commit 77710ff into pybind:master May 9, 2017
@jagerman
Copy link
Member Author

jagerman commented May 9, 2017

Squashed and merged.

Re: descr.h C++14 path and MSVC 2017, it would be cool if someone figured that out, but this change is worthwhile whether or not we do (and makes enabling it for MSVC simple: just change the macro condition in descr.h).

@dean0x7d dean0x7d modified the milestone: v2.2 Aug 13, 2017
@rwgk rwgk mentioned this pull request Feb 9, 2023
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.

None yet

4 participants