Page MenuHomePhabricator

[NFC] Factor out + document build requirements
ClosedPublic

Authored by jfb on Jan 16 2019, 11:30 AM.

Details

Summary

This change factors out compiler checking / warning, and documents LLVM_FORCE_USE_OLD_TOOLCHAIN. It doesn't introduce any functional changes nor policy changes, these will come late.

Diff Detail

Repository
rL LLVM

Event Timeline

jfb created this revision.Jan 16 2019, 11:30 AM

LGTM.

cmake/modules/CheckCompilerVersion.cmake
21 ↗(On Diff #182109)

(I assume the message will be updated later)

61 ↗(On Diff #182109)

(spurious?)

jfb marked 3 inline comments as done.Jan 16 2019, 1:01 PM
jfb added inline comments.
cmake/modules/CheckCompilerVersion.cmake
21 ↗(On Diff #182109)

That's my intent, yes. I'm trying to make the future diff smaller with this patch.

61 ↗(On Diff #182109)

I added it on purpose, it made the flow easier to read IMO. Not that CMake is ever easy or nice to read 😭

smeenai added inline comments.
cmake/modules/CheckCompilerVersion.cmake
11–12 ↗(On Diff #182109)

Did you really mean these to be the same as CLANG_MIN and CLANG_WARN?

17 ↗(On Diff #182109)

You could invert this condition and do an early return instead, which would reduce the remainder of the function by one indent level.

I think this is a good start. That said, this is also a bug-fix (not exactly NFC), since it adds the Clang check to AppleClang.

cmake/modules/CheckCompilerVersion.cmake
11–12 ↗(On Diff #182109)

According to this: https://gist.github.com/yamaya/2924292 those are the same versions. AppleClang 3.1 IS Clang 3.1.

jfb updated this revision to Diff 182136.Jan 16 2019, 1:39 PM
jfb marked an inline comment as done.
  • Use early return.
jfb marked 5 inline comments as done.Jan 16 2019, 1:40 PM
jfb added inline comments.
cmake/modules/CheckCompilerVersion.cmake
11–12 ↗(On Diff #182109)

Right, this is on purpose. The divergence in version numbers came later.

17 ↗(On Diff #182109)

Much nicer, thanks!

jfb edited reviewers, added: rsmith; removed: chandlerc, hans, zturner.Jan 16 2019, 1:41 PM
jyknight accepted this revision.Jan 16 2019, 1:51 PM
This revision is now accepted and ready to land.Jan 16 2019, 1:51 PM
This revision was automatically updated to reflect the committed changes.
jfb marked 2 inline comments as done.
russell.gallop added inline comments.
llvm/trunk/cmake/modules/CheckCompilerVersion.cmake
44

I think that this should report *SIMULATE* VERSION rather than *COMPILER* version. i.e.
message(FATAL_ERROR "Host Clang must have at least -fms-compatibility-version=${MSVC_MIN}, your version is ${CMAKE_CXX_SIMULATE_VERSION}.")

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 7:57 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript