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

Thu Sep 23 09:31:35 PDT 2021

ldionne requested changes to this revision.
ldionne added a comment.

In D110005#3010753 <https://reviews.llvm.org/D110005#3010753>, @phosek wrote:

> In D110005#3010694 <https://reviews.llvm.org/D110005#3010694>, @compnerd wrote:
>
>> 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.
>
> This is exactly the kind of suggestion I was looking for, thanks! I'm fine with either, the only reason why `C_COMPILER_`/`CXX_COMPILER_` might be preferable over simple `COMPILER_` is if there we ever wanted to distinguish between flags supported by C and C++ compiler (in particular if there was a flag that was only supported in one mode but not the other). I'm not sure if that's ever going to be the case?
>
> Do you have any thoughts on variable naming for libraries? We seem to have HAVE_LIBPTHREAD <https://github.com/llvm/llvm-project/blob/4edf46f72a8f3bd9d60628d0c852e8ff91921673/llvm/cmake/config-ix.cmake#L88>, LIBCXX_HAS_PTHREAD_LIB <https://github.com/llvm/llvm-project/blob/4edf46f72a8f3bd9d60628d0c852e8ff91921673/libcxx/cmake/config-ix.cmake#L95> and COMPILER_RT_HAS_LIBPTHREAD <https://github.com/llvm/llvm-project/blob/4edf46f72a8f3bd9d60628d0c852e8ff91921673/compiler-rt/cmake/config-ix.cmake#L142>.

I agree with @compnerd , I think this is pure awesomeness. Also agree with the naming. My take would be:

  CXX_COMPILER_SUPPORTS_<flag>
  C_COMPILER_SUPPORTS_<flag>
  PLATFORM_SUPPORTS_LIB_<library>

Requesting changes so it shows up in my review queue properly, but this LGTM in essence. Also we'll want to get a green CI.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110005