build: Bump MSVC warning level up to W3 #1328

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:230524-w3 changing 2 files +11 −2

hebasto commented at 4:06 PM on May 24, 2023: member

Solves one item in #1235.
in src/CMakeLists.txt:82 in cfbec63ca8 outdated
```
  74 | @@ -75,31 +75,37 @@ elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows")
  75 |  endif()
  76 |  unset(${PROJECT_NAME}_soversion)
  77 |  
  78 | +add_library(secp256k1_warning_suppression INTERFACE)
  79 | +target_compile_options(secp256k1_warning_suppression INTERFACE
  80 | +  # Disable warning C4996 for the usage of a function, class member, variable, or typedef that's marked deprecated.
  81 | +  $<$<C_COMPILER_ID:MSVC>:/wd4996>
  82 | +)
```
real-or-random commented at 7:32 AM on May 25, 2023:

Why treat this special and not add it using try_append_c_flags?

hebasto commented at 7:35 AM on May 25, 2023:

Isn't it better to catch such warnings in non-test/bench code?
real-or-random commented at 8:06 AM on May 25, 2023:

I'm not convinced that this justifies making things more complicated in the build system.

Hm, I just checked. The deprecated warnings here are all the same kind:

src/tests.c(7695,27): warning C4996: 'getenv': This function or variable may be unsafe. Consider using _dupenv_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.

And we really don't want these warnings. We can't use _dupenv_s because it exists only on Windows. We can't use getenv_s because it requires C11 Annex K. But can we just pass /D_CRT_SECURE_NO_WARNINGS instead of /wd4996? That's narrower, and then I think there's really no need to restrict it to test/bench/etc.
hebasto commented at 8:56 AM on May 25, 2023:

Thanks! Updated.
real-or-random commented at 7:32 AM on May 25, 2023: contributor

Concept ACK
build: Level up MSVC warnings 1549db0ca5
hebasto force-pushed on May 25, 2023
hebasto commented at 8:55 AM on May 25, 2023: member

Addressed @real-or-random's #1328 (review).
real-or-random approved
real-or-random commented at 9:40 AM on May 25, 2023: contributor

ACK 1549db0ca5193b8ba5d8f7478d54af2ca4b36c7e

Do you believe it's worth making the - vs / style of passing options consistent? I prefer dashes slightly, but that's just my taste. It doesn't matter at all in the end. Maybe if CMake internally uses / (e.g., in add_compile_definitions), we should just switch to that one.
hebasto commented at 9:53 AM on May 25, 2023: member

... if CMake internally uses / ...

It does.

Do you believe it's worth making the - vs / style of passing options consistent?

Consistency within a particular build system does matter. Not sure about "cross build systems" consistency although.
sipa commented at 11:53 AM on May 26, 2023: contributor

utACK 1549db0ca5193b8ba5d8f7478d54af2ca4b36c7e
real-or-random merged this on May 26, 2023
real-or-random closed this on May 26, 2023
hebasto deleted the branch on May 26, 2023
vmta referenced this in commit e1120c94a1 on Jun 4, 2023
sipa referenced this in commit 901336eee7 on Jun 21, 2023
vmta referenced this in commit 8f03457eed on Jul 1, 2023
hebasto referenced this in commit 270d2b37b8 on Jul 21, 2023
delta1 referenced this in commit 3f32c20932 on Aug 8, 2023
delta1 referenced this in commit 31ac0c1081 on Aug 31, 2023
janus referenced this in commit c4348d88db on Sep 11, 2023
div72 referenced this in commit af627d47c3 on Apr 12, 2025
str4d referenced this in commit 5a6bf5f178 on Jun 4, 2025
Fabcien referenced this in commit 7dc1a63a14 on Mar 3, 2026
Fabcien referenced this in commit 8fbbc9174c on Mar 3, 2026

Contributors

hebasto

real-or-random

sipa

Linked (view graph)

#1235 build: Meta-issue for follow-ups to initial CMake merge (#1113)