Page MenuHomePhabricator
Differential D3786

[CMAKE] Make warnings the same with cmake and autotools
AbandonedPublic
Actions

Authored by Fabien on Jul 31 2019, 14:31.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This avoids getting different warnings when running cmake or autotools.
It moves secp256k1 flags to the appropriated CMakeLists.txt, avoids
flags duplication and enable -Wthread-safety-analysis with cmake.

The leveldb flags have not been updated and cmake still removes some
more warnings than autotools.

Test Plan
make check
ninja check bitcoin-bench

Check there is no difference in the compiler warnings.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_warnings_sync
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7020
Build 12087: Bitcoin ABC Buildbot (legacy)
Build 12086: arc lint + arc unit

Event Timeline

Fabien created this revision.Jul 31 2019, 14:31
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 31 2019, 14:31
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Jul 31 2019, 14:31

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jul 31 2019, 14:37

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7020: Diff 10550.Jul 31 2019, 14:37
deadalnix requested changes to this revision.Aug 1 2019, 06:21

I don't see any point in removing warning for which the codebase is green.

This revision now requires changes to proceed.Aug 1 2019, 06:21
Fabien requested review of this revision.Aug 1 2019, 09:51

There is no real warning removal, but some are moved and so getting narrow-scoped:

  • -Wshadow is no longer duplicated in secp256k1, it remains set at the top level;
  • -Wnested-externs and -Wstrict-prototypes are C-only flags and only apply to secp256k1 with autotools, so I moved them;
  • The main "loss" here can be -Wcast-align, which is not green in the codebase and now applies to secp256k1 only (making it effectively green, just like autotools).
  • There is one additional warning, which is -Wthread-safety-analysis (greened by other diffs), which is already enabled with autotools.

Let me know if you think that moving the warnings to secp256k1 is undesirable, and I'll only keep the -Wthread-safety-analysis change.

deadalnix requested changes to this revision.EditedAug 5 2019, 05:45

If the scope is narrowed, then the warning was removed from the rest of the codebase. This is a net negative.

This revision now requires changes to proceed.Aug 5 2019, 05:45
Fabien abandoned this revision.Aug 6 2019, 09:20

Superseeded by D3798 and D3799

Revision Contents
Changeset List

PathSize
src/
5 lines
secp256k1/
5 lines

Diff 10550

View Options

src/CMakeLists.txt

Loading...
View Options

src/secp256k1/CMakeLists.txt

Loading...