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: set /Zc:__cplusplus and /MP to MSVC only #3139

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

zjyhjqs
Copy link
Contributor

@zjyhjqs zjyhjqs commented Jul 21, 2024

  1. macro __cplusplus is enabled by clang-cl
  2. /MP is not supported by clang-cl (warning -Wunused-command-line-argument will be generated)

see also: https://clang.llvm.org/docs/UsersManual.html#id11

1. macro `__cplusplus` is enabled by clang-cl
2. `/MP` is not supported by clang-cl (warning `-Wunused-command-line-argument` will be generated)
@gabime
Copy link
Owner

gabime commented Jul 21, 2024

I dont get it. MSVC is defined by cmake when using msvc compiler (https://cmake.org/cmake/help/latest/variable/MSVC.html)

@zjyhjqs
Copy link
Contributor Author

zjyhjqs commented Jul 21, 2024

Set to true when the compiler is some version of Microsoft Visual C++ or another compiler simulating the Visual C++ cl command-line syntax.

MSVC is also defined when clang-cl is used.

@@ -161,7 +161,7 @@ if(SPDLOG_BUILD_SHARED OR BUILD_SHARED_LIBS)
endif()
add_library(spdlog SHARED ${SPDLOG_SRCS} ${SPDLOG_ALL_HEADERS})
target_compile_definitions(spdlog PUBLIC SPDLOG_SHARED_LIB)
if(MSVC)
if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
Copy link
Contributor

Choose a reason for hiding this comment

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

This if statement is not necessary because $<CXX_COMPILER_ID:MSVC> appears in the following cmake-generator-expressions.

        target_compile_options(spdlog PUBLIC $<$<AND:$<CXX_COMPILER_ID:MSVC>,$<NOT:$<COMPILE_LANGUAGE:CUDA>>>:/wd4251
                                             /wd4275>)

Copy link
Owner

@gabime gabime Jul 22, 2024

Choose a reason for hiding this comment

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

appears in the following cmake-generator-expressions.

but it is reached only if build shared.

Copy link
Contributor

Choose a reason for hiding this comment

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

but it is reached only if build shared.

I overlooked it. In that sense, we need to set the same option for spdlog::spdlog_header_only.

Copy link
Owner

Choose a reason for hiding this comment

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

perhaps, but it is not directly related to this pr, so i will approve for now

@gabime gabime merged commit 5ebfc92 into gabime:v1.x Jul 22, 2024
8 checks passed
@gabime
Copy link
Owner

gabime commented Jul 22, 2024

Merged. Thanks @zjyhjqs

@zjyhjqs zjyhjqs deleted the fix/clang-cl-flags branch July 26, 2024 14:58
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