build: Bump MSVC warning level up to W3 #1328

1. 
   hebasto commented at 4:06 pm on May 24, 2023: member

   Solves one item in #1235.
2. hebasto cross-referenced this on May 24, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto
3. 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:

   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 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.
4. 
   real-or-random commented at 7:32 am on May 25, 2023: contributor

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

   Addressed @real-or-random’s #1328 (review).
8. real-or-random approved
9. 
   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.

10. 
    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.

11. 
    sipa commented at 11:53 am on May 26, 2023: contributor

    utACK 1549db0ca5193b8ba5d8f7478d54af2ca4b36c7e
12. real-or-random merged this on May 26, 2023
13. real-or-random closed this on May 26, 2023

    ---

14. hebasto deleted the branch on May 26, 2023
15. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
16. sipa referenced this in commit 901336eee7 on Jun 21, 2023
17. vmta referenced this in commit 8f03457eed on Jul 1, 2023
18. hebasto referenced this in commit 270d2b37b8 on Jul 21, 2023
19. jonasnick 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

Contributors
hebasto real-or-random sipa