Page MenuHomePhabricator
Differential D3802

[CMAKE] Refactor the AddCompilerFlags facilities
ClosedPublic
Actions

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rSTAGING5d6c493547fe: [CMAKE] Refactor the AddCompilerFlags facilities
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
Branch
cmake_refactor_compiler_flags
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7052
Build 12151: Bitcoin ABC Buildbot (legacy)
Build 12150: arc lint + arc unit

Event Timeline

Fabien created this revision.Aug 5 2019, 15:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 5 2019, 15:13
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Aug 5 2019, 15:13

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 5 2019, 15:19

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7043: Diff 10609.Aug 5 2019, 15:19
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.

teamcity added a comment.Aug 6 2019, 09:52

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 6 2019, 10:04

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7052: Diff 10632.Aug 6 2019, 10:04
deadalnix accepted this revision.Aug 6 2019, 16:15
This revision is now accepted and ready to land.Aug 6 2019, 16:15
Closed by commit rABC5d6c493547fe: [CMAKE] Refactor the AddCompilerFlags facilities (authored by Fabien). · Explain WhyAug 6 2019, 16:33
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC5d6c493547fe: [CMAKE] Refactor the AddCompilerFlags facilities.
Fabien mentioned this in D3825: [CMAKE] Fix -Wunused-command-line-argument when adding linker flag.Aug 8 2019, 14:39
Fabien mentioned this in rABCc907fdbf9748: [CMAKE] Fix -Wunused-command-line-argument when adding linker flag.Aug 11 2019, 08:05
Fabien added a commit: rSTAGING5d6c493547fe: [CMAKE] Refactor the AddCompilerFlags facilities.Sep 19 2019, 01:39
Fabien mentioned this in rSTAGINGc907fdbf9748: [CMAKE] Fix -Wunused-command-line-argument when adding linker flag.Sep 19 2019, 01:44

Revision Contents
Changeset List

PathSize
cmake/
modules/
88 lines

Diff 10632

View Options

cmake/modules/AddCompilerFlags.cmake

Loading...