Page MenuHomePhabricator

[CMAKE] Refactor the AddCompilerFlags facilities
ClosedPublic

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABC5d6c493547fe: [CMAKE] Refactor the AddCompilerFlags facilities
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.Mon, Aug 5, 15:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Aug 5, 15:13
deadalnix requested changes to this revision.Tue, Aug 6, 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.Tue, Aug 6, 06:24
Fabien added inline comments.Tue, Aug 6, 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.Tue, Aug 6, 09:19

Avoid duplicating the checks.

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