Page MenuHomePhabricator

[CMAKE] Refactor the AddCompilerFlags facilities
ClosedPublic

Authored by Fabien on Aug 5 2019, 15:13.

Details

Summary

This adds a bit of code factorization by adding a pair of functions to
add and remove flags to target variable.
There is no behavior change.

Depends on D3800 and D3801.

Test Plan
mkdir buildcmake && cd buildcmake
cmake -GNinja ..
ninja check

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 5 2019, 15:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 5 2019, 15:13
deadalnix requested changes to this revision.Aug 6 2019, 06:24

Looks good overall, but it looks like there are leftovers.

cmake/modules/AddCompilerFlags.cmake
19 ↗(On Diff #10609)

By convention, private construct are prefixed with _ .

80 ↗(On Diff #10609)

This check is redundant as add_compiler_flags_to_var already does it.

84 ↗(On Diff #10609)

You probably want to call add_compiler_flags_to_var unconditionally here.

This revision now requires changes to proceed.Aug 6 2019, 06:24
Fabien added inline comments.Aug 6 2019, 07:11
cmake/modules/AddCompilerFlags.cmake
19 ↗(On Diff #10609)

I don't want to make it private, It will be used to add debug only flags (see D3803)

Fabien updated this revision to Diff 10632.Aug 6 2019, 09:19

Avoid duplicating the checks.

deadalnix accepted this revision.Aug 6 2019, 16:15
This revision is now accepted and ready to land.Aug 6 2019, 16:15
This revision was automatically updated to reflect the committed changes.