Remove unreferenced boost headers #14718

murrayn · 2018-11-13T12:55:25Z

Building with clang (e.g. on FreeBSD) is very noisy due to -Wthread-safety-analysis warnings regarding boost. This change removes a number of unnecessary boost includes, and silences the rest of the warnings when building with clang. This allows more potentially interesting warnings to surface from the noise.

Tested on FreeBSD 11.2

DrahtBot · 2018-11-13T15:18:47Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#13339 (wallet: Replace %w by wallet name in -walletnotify script by promag)
#12833 ([qt] move QSettings to bitcoin_rw.conf where possible by Sjors)
#11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2018-11-13T16:30:11Z

ACK on removing the unneeded headers. NACK on the verbose annotations to silence warnings. They should instead be fixed in the broken compiler or broken library.

practicalswift · 2018-11-13T16:54:52Z

@murrayn Can you post a few of the warnings? Could it be that you're using an old version of Boost? :-)

ACK on removing the redundant headers

promag · 2018-11-13T17:43:27Z

+1 remove headers.

murrayn · 2018-11-14T00:34:35Z

@murrayn Can you post a few of the warnings? Could it be that you're using an old version of Boost? :-)

@practicalswift I was originally using boost-libs-1.66.0_1, updated to boost-libs-1.68.0_3 with no improvement.

Here is the warning output from compiling one of the files:

https://0bin.net/paste/JAxYnpYsxSSxXz+4#-ObZEDfUOHpeAnguOYhkZMW5xL9Co7ea8E5E+evc9Zv

practicalswift · 2018-11-14T07:13:10Z

@murrayn Could you limit this PR to the header removal only? That change seems to have consensus ACK and will likely be a quick merge :-)

src/init.cpp

maflcko · 2018-11-14T16:06:31Z

ACK c5e6cd459197f0829069641d109b7b7b2f4042f8 (checked that the bitcoind size reduces by about 1%)

PastaPastaPasta

utACK c5e6cd4

ken2812221

Concept ACK.

src/wallet/db.cpp

src/index/txindex.cpp

maflcko

I looked at a few and they seem to be used some others weren't used. Please make sure you only remove unused ones

src/wallet/walletdb.cpp

src/util/time.cpp

src/torcontrol.cpp

src/scheduler.h

src/scheduler.cpp

src/rpc/server.cpp

src/key_io.cpp

maflcko · 2018-11-16T03:44:10Z

utACK c54e5a4

practicalswift · 2018-11-16T07:41:53Z

utACK c54e5a4

c54e5a4 Remove unreferenced boost headers (Murray Nesbitt) Pull request description: Building with clang (e.g. on FreeBSD) is very noisy due to `-Wthread-safety-analysis` warnings regarding boost. This change removes a number of unnecessary boost includes, and silences the rest of the warnings when building with clang. This allows more potentially interesting warnings to surface from the noise. Tested on FreeBSD 11.2 Tree-SHA512: 5e6a0623188b9be59aeae52866799aefb4c3c9ab5e569b07ee8d43fc92e0b5f1f76b96bb54c35c7043148df84641b4a96927fb71f6eb00460c20cd19cf250900

Sjors · 2018-11-19T19:25:51Z

On master:

test/lint/lint-includes.sh
Good job! The Boost dependency "boost/filesystem/fstream.hpp" is no longer used.
Please remove it from EXPECTED_BOOST_INCLUDES in test/lint/lint-includes.sh
to make sure this dependency is not accidentally reintroduced.

I'll make a PR.

laanwj · 2018-11-20T09:02:47Z

src/fs.h

 #endif

 #include <boost/filesystem.hpp>
-#include <boost/filesystem/fstream.hpp>


Next time you're removing a header please check carefully that it's not used; we're also using includes to signal dependencies; so it being included indirectly, somehow, is not a good reason to remove it at the direct use site.

e816b34 revert removal of fstream.hpp header in fs.h (Karl-Johan Alm) Pull request description: We cannot (yet) remove the EXPECTED_BOOST_INCLUDES entry as this header is still needed in `fs.h` (see #14763). Partially reverts #14718. Tree-SHA512: e94d8d6208bee14af20a7a529e60a4898358ec8c070a8bf0701e589a2ae33df1305deac83cee619f103c24be0eb3c12a2f490209c125b247acf21561c7de456e

Summary: c54e5a41c4 Remove unreferenced boost headers (Murray Nesbitt) Pull request description: Building with clang (e.g. on FreeBSD) is very noisy due to `-Wthread-safety-analysis` warnings regarding boost. This change removes a number of unnecessary boost includes, and silences the rest of the warnings when building with clang. This allows more potentially interesting warnings to surface from the noise. Tested on FreeBSD 11.2 Tree-SHA512: 5e6a0623188b9be59aeae52866799aefb4c3c9ab5e569b07ee8d43fc92e0b5f1f76b96bb54c35c7043148df84641b4a96927fb71f6eb00460c20cd19cf250900 Backport of Core [[bitcoin/bitcoin#14718 | PR14718]] and [[bitcoin/bitcoin#14768 | PR14768]] (reverts fs.h changes) Changes to util/system.cpp were ignored because we diverged here: https://reviews.bitcoinabc.org/rSTAGING47162673c79c757a9c038c4ddc41fb3022223bde Test Plan: ninja ninja check Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5523

c54e5a4 Remove unreferenced boost headers (Murray Nesbitt) Pull request description: Building with clang (e.g. on FreeBSD) is very noisy due to `-Wthread-safety-analysis` warnings regarding boost. This change removes a number of unnecessary boost includes, and silences the rest of the warnings when building with clang. This allows more potentially interesting warnings to surface from the noise. Tested on FreeBSD 11.2 Tree-SHA512: 5e6a0623188b9be59aeae52866799aefb4c3c9ab5e569b07ee8d43fc92e0b5f1f76b96bb54c35c7043148df84641b4a96927fb71f6eb00460c20cd19cf250900

e816b34 revert removal of fstream.hpp header in fs.h (Karl-Johan Alm) Pull request description: We cannot (yet) remove the EXPECTED_BOOST_INCLUDES entry as this header is still needed in `fs.h` (see bitcoin#14763). Partially reverts bitcoin#14718. Tree-SHA512: e94d8d6208bee14af20a7a529e60a4898358ec8c070a8bf0701e589a2ae33df1305deac83cee619f103c24be0eb3c12a2f490209c125b247acf21561c7de456e

fanquake added the Refactoring label Nov 13, 2018

murrayn force-pushed the clang_warnings branch 2 times, most recently from 5fa7f28 to c5e6cd4 Compare November 14, 2018 09:07

murrayn changed the title ~~Remove unreferenced boost headers and quieten remaining clang warnings~~ Remove unreferenced boost headers Nov 14, 2018

maflcko reviewed Nov 14, 2018

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

PastaPastaPasta approved these changes Nov 15, 2018

View reviewed changes

ken2812221 reviewed Nov 15, 2018

View reviewed changes

src/wallet/db.cpp Outdated Show resolved Hide resolved

src/index/txindex.cpp Outdated Show resolved Hide resolved

murrayn force-pushed the clang_warnings branch 3 times, most recently from f485779 to 77aab9d Compare November 15, 2018 23:42

maflcko suggested changes Nov 16, 2018

View reviewed changes

murrayn force-pushed the clang_warnings branch from 3603cde to ce28832 Compare November 16, 2018 03:03

maflcko reviewed Nov 16, 2018

View reviewed changes

src/key_io.cpp Outdated Show resolved Hide resolved

Remove unreferenced boost headers

c54e5a4

murrayn force-pushed the clang_warnings branch from e84da53 to c54e5a4 Compare November 16, 2018 03:21

maflcko merged commit c54e5a4 into bitcoin:master Nov 19, 2018

Sjors mentioned this pull request Nov 19, 2018

Add deriveaddresses RPC util method #14667

Merged

Sjors mentioned this pull request Nov 19, 2018

Remove filesystem/fstream from EXPECTED_BOOST_INCLUDES #14763

Closed

kallewoof mentioned this pull request Nov 20, 2018

revert removal of fstream.hpp header in fs.h #14768

Merged

laanwj reviewed Nov 20, 2018

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Remove unreferenced boost headers #14718

Remove unreferenced boost headers #14718

Uh oh!

Conversation

murrayn commented Nov 13, 2018

Uh oh!

DrahtBot commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Nov 13, 2018

Uh oh!

practicalswift commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 13, 2018

Uh oh!

murrayn commented Nov 14, 2018

Uh oh!

practicalswift commented Nov 14, 2018

Uh oh!

Uh oh!

maflcko commented Nov 14, 2018

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

ken2812221 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 16, 2018

Uh oh!

practicalswift commented Nov 16, 2018

Uh oh!

Sjors commented Nov 19, 2018

Uh oh!

laanwj Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

DrahtBot commented Nov 13, 2018 •

edited

Loading

practicalswift commented Nov 13, 2018 •

edited

Loading