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

Fix cmake and MSVC Compiler issues #170

Merged
merged 5 commits into from
Oct 1, 2024

Conversation

d5ch4k
Copy link

@d5ch4k d5ch4k commented Apr 26, 2024

MSVC in cmake has always been a boolean variable ( see https://cmake.org/cmake/help/latest/variable/MSVC.html)
detecting the MSVC Compiler requires therefore 'if (MSVC)' in CMakeLists.txt

further, the MSVC Compiler will always emit '199711L' for __cplusplus, if it is called without the option '/Zc:__cplusplus'
(see https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-170 )

this odd behavior will break any version check in the headers.
e.g. './include/boost/parser/detail/text/detail/begin_end.hpp'
or 'boost/hana/traits.hpp'

therefore 'add_compile_options(/Zc:__cplusplus)' is always required for the MSVC compiler

MPo and others added 2 commits April 26, 2024 13:00
MSVC in cmake has always been a boolean variable ( see https://cmake.org/cmake/help/latest/variable/MSVC.html)
detecting the MSVC Compiler requires therefore "if (MSVC)"

further, the MSVC Compiler will always emit '199711L' for __cplusplus, if it is called without the option '/Zc:__cplusplus'

this odd behavior will break any version check in the headers.
e.g. './include/boost/parser/detail/text/detail/begin_end.hpp' or 'boost/hana/traits.hpp

therefore "add_compile_options(/Zc:__cplusplus)" is always nrequired for the MSVC compiler
@tzlaine
Copy link
Collaborator

tzlaine commented Sep 30, 2024

Thanks for doing this. I only recently learned about add_compile_options(/Zc:__cplusplus).
It is quite simply scandalous.

I'm a little nervous about using CMAKE_CXX_COMPILER_FRONTEND_VARIANT, since a Google of it does not point to the usual CMake doc page, though there are definitely references to it in various places. It appears to be undocumented -- is that right? If so, I think I'd prefer an equivalent PR that just adds the missing variant(s) for WIN32. If it is documented, and I just missed it, I'm all for the current approach, except:

Please don't leave a block of commented-out CMake in your PR. :)

@d5ch4k
Copy link
Author

d5ch4k commented Sep 30, 2024

If I type CMAKE_CXX_COMPILER_FRONTEND_VARIANT into google search, the first hit points to https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER_FRONTEND_VARIANT.html, which I suppose is the official documentation.
I oversaw that it was introduced in version 3.14 which makes the adjustment of cmake_minimum_required(VERSION 3.8...3.20)necessary.

If this is a no-go, then

if (MSVC)
    get_filename_component(CNAME "${CMAKE_CXX_COMPILER}" NAME)
    message(STATUS "Detected MSVC style compiler frontend '" ${CNAME} "'")
    add_definitions(/W3)
    add_compile_options(/Zc:__cplusplus) # not sure, if it necessary for clang-cl
else()
    get_filename_component(CNAME "${CMAKE_CXX_COMPILER}" NAME)
    message(STATUS "Detected GNU style compiler frontend '" ${CNAME} "'")
    add_definitions(-Wall)
endif ()

is the way to go (if we assume, there exist only 2 different command-line styles)

Martin

@tzlaine
Copy link
Collaborator

tzlaine commented Sep 30, 2024

Oh no I didn't mean you needed to revert it to an older-Cmake way of doing things. 3.14 is pretty old. I was just worried that CMAKE_CXX_COMPILER_FRONTEND_VARIANT was an undocumented/internal variable. Now that I see it's official, I'd like to use that version of your patch. Sorry for the confusion.

@d5ch4k
Copy link
Author

d5ch4k commented Oct 1, 2024

I've reverted it and updated the cmake_minimum_requiredlines

Martin

@tzlaine
Copy link
Collaborator

tzlaine commented Oct 1, 2024

Very nice! Thanks again for doing this.

@tzlaine tzlaine merged commit 274e4e3 into boostorg:develop Oct 1, 2024
24 of 26 checks passed
@d5ch4k d5ch4k deleted the Fix_MSVC_issues__in_cmake branch October 1, 2024 19:05
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