Page MenuHomePhabricator

[CMAKE] Improve link flag compiler support detection
ClosedPublic

Authored by Fabien on Aug 1 2019, 19:40.

Details

Summary

The add_linker_flag method is able to prevent from introducing link
time errors, but not warnings. This may cause some noise with clang,
which will issue a -Wunused-command-line-argument warning when an
unknown linker flag is set (then ignore it).

This diff promotes the warning to error so the unused link flag will not
be set.

Test Plan
mkdir buildcmake_osx && cd buildcmake_osx
cmake -GNinja .. -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/OSX.cmake
ninja

Check the following warning disappeared with this patch:

clang: warning: argument unused during compilation: '-pie'
[-Wunused-command-line-argument]

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Aug 1 2019, 19:40
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 1 2019, 19:40
deadalnix requested changes to this revision.Aug 2 2019, 00:00
deadalnix added inline comments.
cmake/modules/AddCompilerFlags.cmake
134 ↗(On Diff #10566)

Why not use remove_compiler_flags ?

This revision now requires changes to proceed.Aug 2 2019, 00:00
deadalnix added inline comments.Aug 2 2019, 00:01
cmake/modules/AddCompilerFlags.cmake
118 ↗(On Diff #10566)

You can add the C++ flag.

Fabien requested review of this revision.Aug 2 2019, 07:10
Fabien added inline comments.
cmake/modules/AddCompilerFlags.cmake
134 ↗(On Diff #10566)

I was hesitant to use CMAKE_CXX_FLAGS. The CMAKE_REQUIRED_FLAGS are dedicated to this use case, and this would avoid the removal of the flag if added to CMAKE_CXX_FLAGS elsewhere.

deadalnix accepted this revision.Aug 5 2019, 05:24
deadalnix added inline comments.
cmake/modules/AddCompilerFlags.cmake
134 ↗(On Diff #10566)

Ok. It still feels like this is doing more or less the same, but on another variable. I think thee is some refactoring that could be done here, considering add_c_compiler_flag, add_cxx_compiler_flag and this are doing VERY similar things, just using a different base variable.

This revision is now accepted and ready to land.Aug 5 2019, 05:24
This revision was automatically updated to reflect the committed changes.