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

Thu Mar 24 05:36:51 PDT 2022

mstorsjo added a reviewer: beanz.
mstorsjo added a subscriber: beanz.
mstorsjo 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 guess the naming of variables was indeed the only main issue to settle here. The currently proposed naming in this patch originally, e.g. `CXX_SUPPORTS_...`, does match what LLVM uses in `add_flag_if_supported` here: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/HandleLLVMOptions.cmake#L260-L265

So in this current form, this patch can also get cache hits between tests from llvm/cmake/modules and the rest of the runtimes, while if we settle on a different naming scheme like `CXX_COMPILER_SUPPORTS_...` we won't get  matches between tests in those different areas. (Not sure if there are any right now, but there could be.) And expanding this patch to rename the naming style of LLVM's cmake modules would grow the scope of this patch quite significantly.

Or what's @beanz 's opinion on it?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110005