Page MenuHomePhabricator

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

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

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