Skip to content

CXX-2629 Remove requirement to set /Zc:__cplusplus in MSVC flags #924

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 12, 2023
Merged

CXX-2629 Remove requirement to set /Zc:__cplusplus in MSVC flags #924

merged 4 commits into from
Jan 12, 2023

Conversation

kevinAlbs
Copy link
Collaborator

@kevinAlbs kevinAlbs commented Jan 10, 2023 •
edited
Loading

Summary

  • Remove requirement for /Zc:__cplusplus

Background & Motivation

The current documented install instructions note:

For building with Visual Studio 2017 (without a C++17 polyfill), it is necessary to configure with an additional option, /Zc:__cplusplus to opt into the correct definition of __cplusplus (problem described here):

'C:\Program Files (x86)\CMake\bin\cmake.exe' .. \
    -G "Visual Studio 15 2017 Win64"            \
    -DCMAKE_CXX_STANDARD=17                     \
    -DCMAKE_CXX_FLAGS="/Zc:__cplusplus"         \
    -DCMAKE_PREFIX_PATH=C:\mongo-c-driver       \
    -DCMAKE_INSTALL_PREFIX=C:\mongo-cxx-driver  \

Setting CMAKE_CXX_FLAGS appears to have the side-effect of overwriting the default MSVC flags set by cmake.
For example, if CMAKE_CXX_FLAGS is not set, this is the output I get with cmake 3.23.1 on my Windows machine:

CMAKE_CXX_FLAGS=/DWIN32 /D_WINDOWS /W3 /GR /EHsc

Following the C++ driver instructions results in CMAKE_CXX_FLAGS only including /Zc:__cplusplus.

Excluding /EHsc appears particularly problematic. /EHsc is the documented default for the exception handling model:

Use an /EH compiler option to specify the exception handling model to use in a C++ project. Standard C++ exception handling (/EHsc) is the default in new C++ projects in Visual Studio.

Not including the /EHsc flag results in warnings during compilation:

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\ostream(305,1): warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc

Setting /EHsc appears to fix an Internal compiler error reported in CXX-2616.

https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ notes:

The _MSVC_LANG predefined macro continues to report the Standard version switch regardless of the value of /Zc:__cplusplus. _MSVC_LANG is set whether or not the /Zc:__cplusplus option is enabled. When /Zc:__cplusplus is enabled, __cplusplus == _MSVC_LANG.

This PR adds a check for _MSVC_LANG, to remove the requirement to pass /Zc:__cplusplus. Following the documentation no longer results in omitting the /EHsc flag.

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2023 •
edited
Loading

Codecov Report

Merging #924 (e8e8a8a) into master (7c95b97) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #924      +/-   ##
==========================================
- Coverage   91.28%   91.28%   -0.01%     
==========================================
  Files         384      384              
  Lines       23324    23324              
==========================================
- Hits        21292    21291       -1     
- Misses       2032     2033       +1
Impacted Files Coverage Δ
src/mongocxx/database.cpp 77.77% <0.00%> (-0.43%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kevinAlbs kevinAlbs changed the title CXX-2055 Set /EHsc when building on Visual Studio CXX-2629 Remove requirement to set /Zc:__cplusplus in MSVC flags Jan 12, 2023
@kevinAlbs kevinAlbs marked this pull request as ready for review January 12, 2023 16:24
eramongodb
eramongodb approved these changes Jan 12, 2023
View reviewed changes
src/bsoncxx/stdx/make_unique.hpp
@@ -48,7 +48,7 @@ using ::boost::make_unique;
BSONCXX_INLINE_NAMESPACE_END
} // namespace bsoncxx

#elif __cplusplus >= 201402L
#elif __cplusplus >= 201402L || (defined(_MSVC_LANG) && _MSVC_LANG >= 201402L)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was concerned about testing __cplusplus before _MSVC_LANG, but it seems Catch2 does this as well, so I believe this should be fine.

vector-of-bool
vector-of-bool approved these changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinAlbs kevinAlbs merged commit 23f9917 into mongodb:master Jan 12, 2023
kevinAlbs added a commit that referenced this pull request Jan 12, 2023
@kevinAlbs
CXX-2629 Remove requirement to set /Zc:__cplusplus in MSVC flags (#924
2b21417 
)

* remove /EHsc from msvc2017 evg flags

* remove requirement for /Zc:__cplusplus

* remove /Zc:__cplusplus from documented requirements

Note option requirements for <= 3.7.0. Note /EHsc is required for <= 3.7.0.

* remove /Zc:__cplusplus from example projects
@eramongodb eramongodb mentioned this pull request Nov 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vector-of-bool vector-of-bool vector-of-bool approved these changes

@eramongodb eramongodb eramongodb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kevinAlbs @codecov-commenter @vector-of-bool @eramongodb