bench: [refactor] iwyu #30716

maflcko · 2024-08-26T15:49:38Z

Missing includes are problematic, because:

Upcoming releases of a C++ standard library implementation often minimize their internal header dependencies. For example, _LIBCPP_REMOVE_TRANSITIVE_INCLUDES (https://libcxx.llvm.org/DesignDocs/HeaderRemovalPolicy.html). This can lead to compile failures, which are easy to fix for developers, but may not be for users. For example, commit 138f867 had to add missing includes to accommodate GCC 15 (and the commit had to be backported).
A Bitcoin Core developer removing a feature from a module and wanting to drop the now unused includes may not be able to do so without touching other unrelated files, because those files rely on the transitive includes.

Moreover, missing or extraneous includes are problematic, because they may be confusing the code reader as to what the real dependencies are.

Finally, extraneous includes may slow down the build.

Fix all issues in bench, by applying the rule include-what-you-use (iwyu).

Follow-up pull requests will handle the other places.

DrahtBot · 2024-08-26T15:49:40Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, hodlinator, hebasto, brunoerg, stickies-v, achow101
Concept ACK	ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

hebasto · 2024-08-26T15:54:58Z

Concept ACK.

Maybe check src/bench in the CI separately and treat warnings as errors?

maflcko · 2024-08-26T15:56:42Z

Maybe check src/bench in the CI separately and treat warnings as errors?

Yes, this will be a follow-up, because it requires other changes as well.

ismaelsadeeq

Concept ACK

hebasto

I still see IWYU warnings in https://cirrus-ci.com/task/5307830952001536. Here are some of them:

bench/addrman.cpp should add these lines:
#include "netaddress.h"     // for CService, CNetAddr, Network
#include "protocol.h"       // for CAddress, ServiceFlags

bench/bech32.cpp should remove these lines:
- #include <cstdint>  // lines 9-9

bench/bench.h should add these lines:
#include "nanobench.h"    // for Bench (ptr only)

bench/data.cpp should add these lines:
#include <iterator>                      // for begin, end

brunoerg · 2024-08-26T21:21:09Z

Concept ACK

maflcko · 2024-08-27T06:44:42Z

I still see IWYU warnings

Thanks, fixed.

TheCharlatan · 2024-08-27T08:33:45Z

Still seems to be:

bench/bench.h should add these lines:
#include "nanobench.h"    // for Bench (ptr only)

bench/bench.h should remove these lines:

The full include-list for bench/bench.h:
#include <bench/nanobench.h>
#include <util/fs.h>      // for path
#include <util/macros.h>  // for PASTE2, STRINGIZE
#include <chrono>         // for milliseconds
#include <cstdint>        // for uint8_t
#include <functional>     // for function
#include <map>            // for map
#include <string>         // for string, operator<=>
#include <utility>        // for pair
#include <vector>         // for vector
#include "nanobench.h"    // for Bench (ptr only)
---

Yes, this will be a follow-up, because it requires other changes as well.

I'm guessing handling this case, or ignoring it, is part of that?

hodlinator · 2024-08-27T08:44:03Z

Concept ACK

Thanks for taking the initiative on this! (Should hopefully free contributors from considering #includes when reviewing, recent example of me doing so: #30571 (review)).

TheCharlatan

ACK fab0e83

hodlinator · 2024-08-27T09:05:07Z

ACK fab0e83

https://cirrus-ci.com/task/4676750536343552 log only shows bench/ issues with bench/bench.h itself, as TheCharlatan previously mentioned.

hebasto · 2024-08-27T09:07:20Z

Still seems to be:

bench/bench.h should add these lines:
#include "nanobench.h"    // for Bench (ptr only)

bench/bench.h should remove these lines:

The full include-list for bench/bench.h:
#include <bench/nanobench.h>
#include <util/fs.h>      // for path
#include <util/macros.h>  // for PASTE2, STRINGIZE
#include <chrono>         // for milliseconds
#include <cstdint>        // for uint8_t
#include <functional>     // for function
#include <map>            // for map
#include <string>         // for string, operator<=>
#include <utility>        // for pair
#include <vector>         // for vector
#include "nanobench.h"    // for Bench (ptr only)
---

Yes, this will be a follow-up, because it requires other changes as well.

I'm guessing handling this case, or ignoring it, is part of that?

It reminds me include-what-you-use/include-what-you-use#1280.

hebasto

ACK fab0e83.

brunoerg

crACK fab0e83

stickies-v

ACK fab0e83

Reviewed by:

checking that all changes are limited to updating includes and fwd declarations in bench/ files
checking the clang-tidy CI job output for suggestions on bench/ files, yielding only one result for bench/bench.h (as already pointed out by other reviewers):

grep "bench\/.*should" -A 3
bench/bench.h should add these lines:
#include "nanobench.h"    // for Bench (ptr only)

bench/bench.h should remove these lines:

The full include-list for bench/bench.h:
#include <bench/nanobench.h>

stickies-v · 2024-08-27T14:30:17Z

src/bench/bench.cpp

@@ -4,16 +4,21 @@

 #include <bench/bench.h>

-#include <test/util/setup_common.h>
+#include <test/util/setup_common.h> // IWYU pragma: keep


Review note: this include (and thus the new pragma) is necessary to compile setup_common.cpp. Without it, fails with

Undefined symbols for architecture arm64: "_G_TEST_COMMAND_LINE_ARGUMENTS", referenced from: BasicTestingSetup::BasicTestingSetup(ChainType, TestOpts) in libtest_util.a[11](libtest_util_a-setup_common.o) "_G_TEST_GET_FULL_NAME", referenced from: BasicTestingSetup::BasicTestingSetup(ChainType, TestOpts) in libtest_util.a[11](libtest_util_a-setup_common.o) "_G_TEST_LOG_FUN", referenced from: BasicTestingSetup::BasicTestingSetup(ChainType, TestOpts) in libtest_util.a[11](libtest_util_a-setup_common.o) ld: symbol(s) not found for architecture arm64

achow101 · 2024-08-27T17:43:35Z

ACK fab0e83

DrahtBot added the Tests label Aug 26, 2024

ismaelsadeeq reviewed Aug 26, 2024

View reviewed changes

hebasto reviewed Aug 26, 2024

View reviewed changes

bench: [refactor] iwyu

fab0e83

maflcko force-pushed the 2408-iwyu branch from fa3e7d0 to fab0e83 Compare August 27, 2024 05:37

TheCharlatan approved these changes Aug 27, 2024

View reviewed changes

DrahtBot requested review from brunoerg, ismaelsadeeq and hebasto August 27, 2024 08:54

hebasto approved these changes Aug 27, 2024

View reviewed changes

brunoerg approved these changes Aug 27, 2024

View reviewed changes

stickies-v approved these changes Aug 27, 2024

View reviewed changes

achow101 merged commit dc90542 into bitcoin:master Aug 27, 2024
16 checks passed

maflcko deleted the 2408-iwyu branch August 29, 2024 06:31

bitcoin locked and limited conversation to collaborators Aug 29, 2025

bench: [refactor] iwyu #30716

bench: [refactor] iwyu #30716

Uh oh!

Conversation

maflcko commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

hebasto commented Aug 26, 2024

Uh oh!

maflcko commented Aug 26, 2024

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Aug 26, 2024

Uh oh!

maflcko commented Aug 27, 2024

Uh oh!

TheCharlatan commented Aug 27, 2024

Uh oh!

hodlinator commented Aug 27, 2024

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

hodlinator commented Aug 27, 2024

Uh oh!

hebasto commented Aug 27, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 27, 2024

Uh oh!

Uh oh!

Uh oh!

maflcko commented Aug 26, 2024 •

edited

Loading

DrahtBot commented Aug 26, 2024 •

edited

Loading