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.

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

cmake: Adjust diagnostic flags for clang-cl #1647

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

cmake: Adjust diagnostic flags for clang-cl #1647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84c0bd1
cmake: Adjust diagnostic flags for clang-cl
hebasto Dec 13, 2024
4c50d73
ci: Add new "Windows (clang-cl)" job
hebasto Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,8 @@ jobs:
cpp_flags: '/DSECP256K1_MSVC_MULH_TEST_OVERRIDE'
- job_name: 'x86 (MSVC): Windows (VS 2022)'
cmake_options: '-A Win32'
- job_name: 'x64 (MSVC): Windows (clang-cl)'
cmake_options: '-T ClangCL'

steps:
- name: Checkout
Expand Down
12 changes: 8 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,17 +242,21 @@ endif()

include(TryAppendCFlags)
if(MSVC)
# Keep the following commands ordered lexicographically.
# For both cl and clang-cl compilers.
try_append_c_flags(/W3) # Production quality warning level.
# Eliminate deprecation warnings for the older, less secure functions.
add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
else()
try_append_c_flags(-Wall) # GCC >= 2.95 and probably many other compilers.
endif()
if(CMAKE_C_COMPILER_ID STREQUAL "MSVC")
# Keep the following commands ordered lexicographically.
Copy link
Contributor

@theuni theuni Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd prefer to set /W3 here and -Wall with the other try_append_c_flags for consistency, as I don't see why it needs to be special-cased.

Edit: Retracted, see below.

Copy link
Member Author

Choose a reason for hiding this comment

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

According to the docs, the /Wall option in clang-cl, which is equivalent to -Wall, enables Clang's core -Weverything option.

We use /W3, which enables Clang's core -Wall option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. Confirmed: https://godbolt.org/z/WEr3qGeox

That's... hugely unexpected. It really doesn't give me much faith in the proper forwarding of other options :(

try_append_c_flags(/wd4146) # Disable warning C4146 "unary minus operator applied to unsigned type, result still unsigned".
try_append_c_flags(/wd4244) # Disable warning C4244 "'conversion' conversion from 'type1' to 'type2', possible loss of data".
try_append_c_flags(/wd4267) # Disable warning C4267 "'var' : conversion from 'size_t' to 'type', possible loss of data".
# Eliminate deprecation warnings for the older, less secure functions.
add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
else()
# Keep the following commands ordered lexicographically.
try_append_c_flags(-pedantic)
try_append_c_flags(-Wall) # GCC >= 2.95 and probably many other compilers.
try_append_c_flags(-Wcast-align) # GCC >= 2.95.
try_append_c_flags(-Wcast-align=strict) # GCC >= 8.0.
try_append_c_flags(-Wconditional-uninitialized) # Clang >= 3.0 only.
Expand Down
Loading