bench: [refactor] iwyu

maflcko commented at 3:49 pm on August 26, 2024: member

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 138f8671569f7ebb8c84e9d80c44cddeda9e3845 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 commented at 3:49 pm on August 26, 2024: contributor

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.

DrahtBot added the label Tests on Aug 26, 2024

hebasto commented at 3:54 pm on August 26, 2024: member

Concept ACK.

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

maflcko commented at 3:56 pm on August 26, 2024: member

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 commented at 4:50 pm on August 26, 2024: member

Concept ACK

hebasto commented at 5:39 pm on August 26, 2024: member

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

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

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

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

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

brunoerg commented at 9:21 pm on August 26, 2024: contributor

Concept ACK

bench: [refactor] iwyu fab0e834b8

maflcko force-pushed on Aug 27, 2024

maflcko commented at 6:44 am on August 27, 2024: member

I still see IWYU warnings

Thanks, fixed.

TheCharlatan commented at 8:33 am on August 27, 2024: contributor

Still seems to be:

 0bench/bench.h should add these lines:
 1#include "nanobench.h"    // for Bench (ptr only)
 2
 3bench/bench.h should remove these lines:
 4
 5The full include-list for bench/bench.h:
 6#include <bench/nanobench.h>
 7#include <util/fs.h>      // for path
 8#include <util/macros.h>  // for PASTE2, STRINGIZE
 9#include <chrono>         // for milliseconds
10#include <cstdint>        // for uint8_t
11#include <functional>     // for function
12#include <map>            // for map
13#include <string>         // for string, operator<=>
14#include <utility>        // for pair
15#include <vector>         // for vector
16#include "nanobench.h"    // for Bench (ptr only)
17---

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 commented at 8:44 am on August 27, 2024: contributor

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#pullrequestreview-2253654936).

TheCharlatan approved

TheCharlatan commented at 8:54 am on August 27, 2024: contributor

ACK fab0e834b8cf906c286ebbeed86eca8d65854f75

DrahtBot requested review from brunoerg on Aug 27, 2024

DrahtBot requested review from ismaelsadeeq on Aug 27, 2024

DrahtBot requested review from hebasto on Aug 27, 2024

hodlinator commented at 9:05 am on August 27, 2024: contributor

ACK fab0e834b8cf906c286ebbeed86eca8d65854f75

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

hebasto commented at 9:07 am on August 27, 2024: member

Still seems to be:

 0bench/bench.h should add these lines:
 1#include "nanobench.h"    // for Bench (ptr only)
 2
 3bench/bench.h should remove these lines:
 4
 5The full include-list for bench/bench.h:
 6#include <bench/nanobench.h>
 7#include <util/fs.h>      // for path
 8#include <util/macros.h>  // for PASTE2, STRINGIZE
 9#include <chrono>         // for milliseconds
10#include <cstdint>        // for uint8_t
11#include <functional>     // for function
12#include <map>            // for map
13#include <string>         // for string, operator<=>
14#include <utility>        // for pair
15#include <vector>         // for vector
16#include "nanobench.h"    // for Bench (ptr only)
17---

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 https://github.com/include-what-you-use/include-what-you-use/issues/1280.

hebasto approved

hebasto commented at 9:10 am on August 27, 2024: member

ACK fab0e834b8cf906c286ebbeed86eca8d65854f75.

brunoerg approved

brunoerg commented at 12:00 pm on August 27, 2024: contributor

crACK fab0e834b8cf906c286ebbeed86eca8d65854f75

in src/bench/bench.cpp:7 in fab0e834b8

3@@ -4,16 +4,21 @@
4 
5 #include <bench/bench.h>
6 
7-#include <test/util/setup_common.h>
8+#include <test/util/setup_common.h> // IWYU pragma: keep

stickies-v commented at 2:30 pm on August 27, 2024:

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

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

stickies-v approved

stickies-v commented at 2:39 pm on August 27, 2024: contributor

ACK fab0e834b8cf906c286ebbeed86eca8d65854f75

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):

0grep "bench\/.*should" -A 3
1bench/bench.h should add these lines:
2#include "nanobench.h"    // for Bench (ptr only)
3
4bench/bench.h should remove these lines:
5
6The full include-list for bench/bench.h:
7#include <bench/nanobench.h>

achow101 commented at 5:43 pm on August 27, 2024: member

ACK fab0e834b8cf906c286ebbeed86eca8d65854f75

achow101 merged this on Aug 27, 2024

achow101 closed this on Aug 27, 2024

maflcko deleted the branch on Aug 29, 2024

bench: [refactor] iwyu #30716

Code Coverage

Reviews