Page MenuHomePhabricator

[WIP][CMake] Unify variable names
Needs RevisionPublic

Authored by phosek on Sep 17 2021, 2:38 PM.

Details

Reviewers
MaskRay
compnerd
ldionne
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Summary

Avoid repeating CMake checks across runtimes by unifying names of
variables used for results to leverage CMake caching.

Diff Detail

Event Timeline

phosek created this revision.Sep 17 2021, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2021, 2:38 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
phosek requested review of this revision.Sep 17 2021, 2:38 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptSep 17 2021, 2:38 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: Restricted Project. · View Herald Transcript
MaskRay accepted this revision.Sep 17 2021, 4:05 PM
MaskRay added a subscriber: MaskRay.

Looks great to me. Looks like CXX_SUPPORTS_ is already used.

mstorsjo added inline comments.
libcxxabi/cmake/config-ix.cmake
85

The newly added checks in llvm-project/runtimes (for -nostdinc++ and -nostdlib++) probably should be aligned with these too.

compnerd requested changes to this revision.Mon, Sep 20, 3:20 PM
compnerd added a subscriber: compnerd.

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.

compiler-rt/CMakeLists.txt
441

Can we rename this? I don't think that C_SUPPORTS_NODEFAULTLIBS_FLAG is ideal - how about COMPILER_SUPPORTS_... or C_COMPILER_...?

compiler-rt/cmake/config-ix.cmake
57

Similar, COMPILER_SUPPORTS_ or C_COMPILER_SUPPORTS_.... The flag isn't about the C library

58

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.

This revision now requires changes to proceed.Mon, Sep 20, 3:20 PM

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, LIBCXX_HAS_PTHREAD_LIB and COMPILER_RT_HAS_LIBPTHREAD.

ldionne requested changes to this revision.Thu, Sep 23, 9:31 AM
ldionne added a subscriber: ldionne.

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, LIBCXX_HAS_PTHREAD_LIB and COMPILER_RT_HAS_LIBPTHREAD.

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.