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: memberSolves one item in #1235.
-
hebasto cross-referenced this on May 24, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto
-
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 usingtry_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:
0src/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 usegetenv_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.
real-or-random commented at 7:32 am on May 25, 2023: contributorConcept ACKbuild: Level up MSVC warnings 1549db0ca5hebasto force-pushed on May 25, 2023hebasto commented at 8:55 am on May 25, 2023: memberAddressed @real-or-random’s #1328 (review).real-or-random approvedreal-or-random commented at 9:40 am on May 25, 2023: contributorACK 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., inadd_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: contributorutACK 1549db0ca5193b8ba5d8f7478d54af2ca4b36c7ereal-or-random merged this on May 26, 2023real-or-random closed this on May 26, 2023
hebasto deleted the branch on May 26, 2023vmta referenced this in commit e1120c94a1 on Jun 4, 2023sipa referenced this in commit 901336eee7 on Jun 21, 2023vmta referenced this in commit 8f03457eed on Jul 1, 2023hebasto referenced this in commit 270d2b37b8 on Jul 21, 2023jonasnick cross-referenced this on Jul 26, 2023 from issue Upstream PRs 1314, 1317, 1318, 1316, 1327, 1310, 1328, 1333, 1330, 1334, 1337, 1341, 1339, 1350, 1349, 1338, 1129, 1347, 1336, 1295, 1354, 1355, 1356 by jonasnick
This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 01:15 UTC
More mirrored repositories can be found on mirror.b10c.me