This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Don't trust compiler error code, also check for errors
ClosedPublic

Authored by beanz on Aug 1 2016, 1:04 PM.

Details

Summary

rnk reported that MSVC ignores unknown flags and still returns 0. This should cause unknown flags to be an error during the compiler check.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 66363.Aug 1 2016, 1:04 PM
beanz retitled this revision from to [CMake] Don't trust compiler error code, also check for errors.
beanz updated this object.
beanz added a reviewer: rnk.
beanz added a subscriber: llvm-commits.
rnk added inline comments.Aug 1 2016, 1:11 PM
cmake/Modules/BuiltinTests.cmake
1 ↗(On Diff #66363)

That was also my first thought, but the file has this comment:

# Do NOT include this module directly into any of your code. It is meant as
# a library for Check*CompilerFlag.cmake modules. It's content may change in
# any way between releases.

Think we should do it anyway?

44 ↗(On Diff #66363)

I think you meant if ("${var}" STREQUAL "FAIL_REGEX")

beanz added a subscriber: brad.king.Aug 1 2016, 1:18 PM
beanz added inline comments.
cmake/Modules/BuiltinTests.cmake
1 ↗(On Diff #66363)

My gut is to include it anyway. The alternative is to mimic what it does. @brad.king might have thoughts. Despite saying that, the macro's structure hasn't really changed ever.

44 ↗(On Diff #66363)

Duh...

beanz updated this revision to Diff 66367.Aug 1 2016, 1:19 PM

Updates based on feedback from rnk.

rnk accepted this revision.Aug 1 2016, 1:23 PM
rnk edited edge metadata.

Thanks for dealing with that!

cmake/Modules/BuiltinTests.cmake
2 ↗(On Diff #66367)

We can duplicate the relevant parts when and if it breaks.

This revision is now accepted and ready to land.Aug 1 2016, 1:23 PM
This revision was automatically updated to reflect the committed changes.