[libcxx-commits] [PATCH] D110005: [WIP][CMake] Unify variable names

Mon Sep 20 15:20:08 PDT 2021

compnerd requested changes to this revision.
compnerd added a comment.
This revision now requires changes to proceed.

While I think that the direction of this change is amazing, I'm less enthusiastic about the naming.  I think that it is particularly confusing for the flags to use `C` and `CXX` as the prefixes for the flags as we have llvm-libc (aka "c") and libc++ (aka "c++", "cxx").  Using a more verbose name of `COMPILER_` or `C_COMPILER_`, `CXX_COMPILER_` would allow the flags to be easily attributed to the compiler or the runtime.  However, the C standard flags definitely should be dropped.

================
Comment at: compiler-rt/CMakeLists.txt:441
 # Set common link flags.
-append_list_if(COMPILER_RT_HAS_NODEFAULTLIBS_FLAG -nodefaultlibs SANITIZER_COMMON_LINK_FLAGS)
+append_list_if(C_SUPPORTS_NODEFAULTLIBS_FLAG -nodefaultlibs SANITIZER_COMMON_LINK_FLAGS)
 append_list_if(COMPILER_RT_HAS_Z_TEXT -Wl,-z,text SANITIZER_COMMON_LINK_FLAGS)
----------------
Can we rename this?  I don't think that `C_SUPPORTS_NODEFAULTLIBS_FLAG` is ideal - how about `COMPILER_SUPPORTS_...` or `C_COMPILER_...`?

================
Comment at: compiler-rt/cmake/config-ix.cmake:57
 # CodeGen options.
-check_c_compiler_flag(-ffreestanding         COMPILER_RT_HAS_FFREESTANDING_FLAG)
-check_c_compiler_flag(-std=c11               COMPILER_RT_HAS_STD_C11_FLAG)
+check_c_compiler_flag(-ffreestanding         C_SUPPORTS_FFREESTANDING_FLAG)
+check_c_compiler_flag(-std=c11               C_SUPPORTS_STD_C11_FLAG)
----------------
Similar, `COMPILER_SUPPORTS_` or `C_COMPILER_SUPPORTS_...`.  The flag isn't about the C library

================
Comment at: compiler-rt/cmake/config-ix.cmake:58
+check_c_compiler_flag(-ffreestanding         C_SUPPORTS_FFREESTANDING_FLAG)
+check_c_compiler_flag(-std=c11               C_SUPPORTS_STD_C11_FLAG)
 check_cxx_compiler_flag(-fPIC                COMPILER_RT_HAS_FPIC_FLAG)
----------------
I think that we should get rid of this check entirely.  Instead, we should use the `C_STANDARD` and `C_STANDARD_REQUIRED` properties on the targets.  This has been available since CMake 3.1.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110005/new/

https://reviews.llvm.org/D110005