Fix build for MSVC 19.42 #377
Conversation
There was a problem hiding this comment.
@AVAtarMod Thank you for sending the PR.
I set some of compiler options for MSVC on CI.
See
async_mqtt/.github/workflows/gha.yml
Line 178 in 5243283
They should be defined to build separate compilation library too.
/Zc:preprocessor /bigobj have already been added in your PR. Could you add the rest of options?
lib/CMakeLists.txt
Outdated
|
|
||
| get_property(is_multi_config GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG) | ||
| if(is_multi_config) | ||
| if ($<STREQUAL:$<CXX_COMPILER_ID>,"MSVC">) |
There was a problem hiding this comment.
| if ($<STREQUAL:$<CXX_COMPILER_ID>,"MSVC">) | |
| if (MSVC) |
async_mqtt uses MSVC at other place. So if you can use it here, please use it.
See https://cmake.org/cmake/help/latest/variable/MSVC.html
There was a problem hiding this comment.
I remove this multi-config check.
lib/CMakeLists.txt
Outdated
| macro(fix_msvc_build) | ||
| message(STATUS "MSVC build fix applied") | ||
| target_compile_options(async_mqtt_separate PRIVATE "/Zc:preprocessor" "/bigobj") | ||
| endmacro() | ||
|
|
||
| get_property(is_multi_config GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG) | ||
| if(is_multi_config) |
There was a problem hiding this comment.
It seems that this part is for the different way to get CMAKE_CXX_COMPILER_ID.
I am not 100% sure but MSVC could be used for both, if it is right, this part of code could be removed.
And the contents of the macro could be moved into if (MSVC) part.
There was a problem hiding this comment.
Yes, MSVC could be used, but as far as I know, this is legacy method
There was a problem hiding this comment.
Oh really, I will update exsiting if (MSVC) code later.
Why we need |
I realized I was mistaken. I now believe these options should be left to the user's discretion. If users want to enable them, they can pass them as CMake arguments via CMAKE_CXX_FLAGS. You've already chosen the minimal mandatory options. Please keep the original code unchanged in this regard. |
|
Merged. Thank you for your contribution. |
I cannot build async_mqtt without this fix. It may not work at old MSVC version, but I could not find information about version of MSVC which adds used options.