Page MenuHomePhabricator

build: remove `LLVM_CXX_STD` extension point
ClosedPublic

Authored by compnerd on Oct 24 2019, 2:25 PM.

Details

Reviewers
beanz
xiaobai
jfb
Summary

This extension point is not needed. Provide the equivalent option LLVM_ENABLE_CXX1Z which mirrors the previous extension point. Rely on CMake to provide the check for the compiler instead.

Diff Detail

Repository
rL LLVM

Event Timeline

compnerd created this revision.Oct 24 2019, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2019, 2:25 PM
xiaobai added inline comments.Oct 24 2019, 2:28 PM
CMakeLists.txt
57

Should be 14 no?

compnerd updated this revision to Diff 226331.Oct 24 2019, 2:31 PM
compnerd marked an inline comment as done.

11 -> 14

xiaobai added a comment.EditedOct 24 2019, 2:37 PM

Why do you want LLVM_CXX_STD to be removed? I suppose somebody will eventually add LLVM_ENABLE_CXX2A, which will require additional logic to ensure that somebody doesn't set LLVM_ENABLE_CXX2A and LLVM_ENABLE_CXX1Z at the same time. I think this can be avoided by removing LLVM_ENABLE_CXX1Z entirely and going through LLVM_CXX_STD entirely.

Why do you want LLVM_CXX_STD to be removed? I suppose somebody will eventually add LLVM_ENABLE_CXX2A, which will require additional logic to ensure that somebody doesn't set LLVM_ENABLE_CXX2A and LLVM_ENABLE_CXX1Z at the same time. I think this can be avoided by removing LLVM_ENABLE_CXX1Z entirely and going through LLVM_CXX_STD entirely.

Can we avoid having any custom variables for this at all and just use CMAKE_CXX_STANDARD? People can do -DCMAKE_CXX_STANDARD=17 or whatever else they want.

compnerd updated this revision to Diff 226340.Oct 24 2019, 2:59 PM

Use CMAKE_CXX_STANDARD as the variable instead of a custom variable.

My reasoning behind that was that it seemed that some people were concerned about cached variables not being easy to remove in subsequent updates. This will now default to 14, but allow the user to specify a different value.

smeenai added a reviewer: jfb.Oct 24 2019, 3:05 PM

The change itself looks fine, but idk if we need to preserve support for any of the older variables (LLVM_ENABLE_CXX1Z in particular). Idk if there's an easy way to e.g. audit bot configurations to see what variables they're using ... maybe posting to llvm-dev would be a good idea?

zorg should have the configuration that the bots are using.

Everything about this change seems good to me, the only thing I'd suggest is that we add a documentation note about CMAKE_CXX_STANDARD to docs/CMake.rst.

beanz accepted this revision.Oct 25 2019, 10:28 AM
This revision is now accepted and ready to land.Oct 25 2019, 10:28 AM
compnerd closed this revision.Oct 27 2019, 8:36 PM

2724d9e12960cc1d93eeabbfc9aa1bffffa041cc (with documentation)