cmake: Adjust diagnostic flags for clang-cl #1647

1. 
   hebasto commented at 9:50 am on December 13, 2024: member

   When building with clang-cl on Windows, the output is cluttered with warning messages because compiler diagnostic flags are not applied correctly:

    0> cmake -B build -G Ninja -DCMAKE_C_COMPILER="C:\Users\hebasto\Downloads\clang+llvm-18.1.8-x86_64-pc-windows-msvc\bin\clang-cl.exe"
    1> cmake --build build
    2[1/16] Building C object src\CMakeFiles\bench.dir\bench.c.obj
    3In file included from C:\Users\hebasto\secp256k1\src\bench.c:11:
    4C:\Users\hebasto\secp256k1\src\util.h(34,13): warning: unused function 'print_buf_plain' [-Wunused-function]
    5   34 | static void print_buf_plain(const unsigned char *buf, size_t len) {
    6      |             ^~~~~~~~~~~~~~~
    71 warning generated.
    8[2/16] Building C object src\CMakeFiles\secp256k1_precomputed.dir\precomputed_ecmult_gen.c.obj
    9In file included from C:\Users\hebasto\secp256k1\src\precomputed_ecmult_gen.c:3:
   10In file included from C:\Users\hebasto\secp256k1\src\group.h:10:
   11In file included from C:\Users\hebasto\secp256k1\src\field.h:10:
   12C:\Users\hebasto\secp256k1\src\util.h(34,13): warning: unused function 'print_buf_plain' [-Wunused-function]
   13   34 | static void print_buf_plain(const unsigned char *buf, size_t len) {
   14      |             ^~~~~~~~~~~~~~~
   

   This PR resolves this issue.

   ---

   Additional note for reviewers: The VS builtin clang can also be used assuming that the following VS components are installed:

   

   The user can generate a build system on Windows as follows:

   • Using the default “Visual Studio” generator:

   0cmake -B build -T ClangCL
   

   • Using the “Ninja” generator:

   0cmake -B build -G Ninja -DCMAKE_C_COMPILER=clang-cl
   

   ---

   Required for downstream projects which aim to build with clang-cl (see https://github.com/bitcoin/bitcoin/issues/31456).

2. cmake: Adjust diagnostic flags for clang-cl 84c0bd1f72
3. ci: Add new "Windows (clang-cl)" job 4c50d73dd9
4. real-or-random added the label ci on Dec 13, 2024
5. real-or-random added the label feature on Dec 13, 2024
6. real-or-random added the label build on Dec 13, 2024
7. 
   real-or-random commented at 3:09 pm on December 13, 2024: contributor

   I guess the patch is trivial enough not to bother, but I’m not entirely sure if I understand the purpose of this. Couldn’t you build using clang on Windows directly? Or is the purpose of this PR to help users who want to use Visual Studio but with clang instead of MSVC? My thinking is that if you use Visual Studio for an application that depends on libsecp256k1, you don’t bother how libsecp256k1 itself is built.
8. hebasto renamed this:
   cmake: Enable compiling with `clang-cl`
   cmake: Adjust diagnostic flags for `clang-cl`
   on Dec 13, 2024
9. 
   hebasto commented at 4:03 pm on December 13, 2024: member

   I guess the patch is trivial enough not to bother, but I’m not entirely sure if I understand the purpose of this. Couldn’t you build using clang on Windows directly? Or is the purpose of this PR to help users who want to use Visual Studio but with clang instead of MSVC? My thinking is that if you use Visual Studio for an application that depends on libsecp256k1, you don’t bother how libsecp256k1 itself is built.

   My apologies for any confusion. The PR description has been reworked.

10. real-or-random approved
11. 
    real-or-random commented at 5:33 pm on December 14, 2024: contributor

    utACK 4c50d73dd98edc0b968525a291cf14b708bbdbf1
12. in CMakeLists.txt:253 in 4c50d73dd9

    249+  add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
    250+else()
    251+  try_append_c_flags(-Wall) # GCC >= 2.95 and probably many other compilers.
    252+endif()  
    253+if(CMAKE_C_COMPILER_ID STREQUAL "MSVC")
    254+  # Keep the following commands ordered lexicographically.
    

    ---

    ---

    

    theuni commented at 7:40 pm on December 16, 2024:

    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.

    ---

    

    hebasto commented at 9:15 pm on December 16, 2024:

    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.

    ---

    

    theuni commented at 5:43 pm on December 17, 2024:

    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 :(

13. 
    theuni commented at 7:41 pm on December 16, 2024: contributor

    utACK other than the nit.
14. gatleas17 approved

Contributors
hebasto real-or-random theuni gatleas17

Labels
ci feature build