test: clean up threadpool test types #35174

pull l0rinc wants to merge 2 commits into bitcoin:master from l0rinc:l0rinc/threadpool-test-casts changing 1 files +18 −18

l0rinc commented at 6:28 PM on April 28, 2026: contributor

Loosely related to the review of #31132 (review).

Problem

threadpool_tests currently stores a container-sized task count as int32_t in the range-submission test, then casts it back to size_t when sizing containers and checking future counts.

Some atomic declarations in the same file also use the longer std::atomic<T> spelling while nearby code uses the shorter standard typedefs.

Fix

Use size_t for the task count, generated range, task return type, and accumulators so the test stays in the same type domain as the containers it exercises.

Also switch the remaining simple atomic declarations in threadpool_tests to the matching standard typedefs such as std::atomic_int, std::atomic_bool, and std::atomic_size_t.

test: keep threadpool task count as `size_t`

`threadpool_tests` uses a small task count to size containers and compare future counts, but stores it in `int32_t` and casts it back to `size_t` throughout the range-submission test.

Use `size_t` for the task count, generated range, task return type, and accumulators so the test stays in the container size domain.

b5f8a03738

test: unify `std::atomic_` references in `threadpool_tests` dc4588c129
DrahtBot added the label Tests on Apr 28, 2026
DrahtBot commented at 6:28 PM on April 28, 2026: contributor



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



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35174.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK ferminquant

Concept ACK winterrdog

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
winterrdog commented at 9:52 PM on May 4, 2026: none

concept ACK

the risk of signedness bugs is low given current test parameters, but using size_t in submit_range_of_tasks_complete_successfully provides compile-time type safety for future changes; a worthwhile defensive improvement IMHO
ferminquant commented at 2:05 AM on May 14, 2026: none
ACK dc4588c1296873070f0d6d2c2deb9318df0648a9

Reviewed the diff. The size_t changes keep the range-submission test in the same type domain as the containers and future counts, and the atomic typedef changes are equivalent cleanups.

Ran:
- git diff --check origin/master...HEAD
- cmake --build build --target test_bitcoin -j 8
- ./build/bin/test_bitcoin --run_test=threadpool_tests/submit_range_of_tasks_complete_successfully
- ./build/bin/test_bitcoin --run_test=threadpool_tests
- ctest --test-dir build -R '^threadpool_tests$' --output-on-failure
- valgrind --error-exitcode=1 --quiet ./build/bin/test_bitcoin --run_test=threadpool_tests/submit_range_of_tasks_complete_successfully
in src/test/threadpool_tests.cpp:463 in dc4588c129
```
 459 | @@ -460,30 +460,30 @@ BOOST_AUTO_TEST_CASE(stop_active_wait_drains_queue)
 460 |  // Test 14, submit range of tasks in one lock acquisition
 461 |  BOOST_AUTO_TEST_CASE(submit_range_of_tasks_complete_successfully)
 462 |  {
 463 | -    constexpr int32_t num_tasks{50};
 464 | +    constexpr size_t num_tasks{50};
```
maflcko commented at 7:36 AM on May 14, 2026:

not sure about this. size_t is an arch-dependent type, however 50 is hard-coded and not arch-dependent. I just find it confusing, tedious, and pointless having to think about two arches while reading code or reviewing.

So uint32_t would be a better choice here, because either the test works on 32-bit, or it does not. If it does not, then at least it will also be a failure on 64-bit.

However, the rule is to use signed types for arithmetic stuff, so int32_t was already correct here.

Type	Reviewers
ACK	ferminquant
Concept ACK	winterrdog

in src/test/threadpool_tests.cpp:406 in dc4588c129

 402 | @@ -403,7 +403,7 @@ BOOST_AUTO_TEST_CASE(queued_tasks_complete_after_interrupt)
 403 |      const auto blocking_tasks = BlockWorkers(threadPool, blocker, NUM_WORKERS_DEFAULT);
 404 |  
 405 |      // Queue tasks while all workers are busy, then interrupt
 406 | -    std::atomic<int> counter{0};
 407 | +    std::atomic_int counter{0};

maflcko commented at 7:37 AM on May 14, 2026:

not sure about this. They are exact type aliases. If we wanted to save one char for all of those, it should be a linter. But I think one-off touchign code for this doesn't seem worth it.

maflcko commented at 7:41 AM on May 14, 2026: member

Not sure. This is shuffling some types, the pull description (llm generated?) is almost longer than the diff, the linked comment in the pull description doesn't work (not your fault, I think GitHub is wholesale broken now), and I think the patch goes in the wrong direction
l0rinc closed this on May 14, 2026
l0rinc deleted the branch on May 14, 2026
maflcko commented at 1:00 PM on May 14, 2026: member
the linked comment in the pull description doesn't work (not your fault, I think GitHub is wholesale broken now)

For reference, the mirror still works. E.g
- https://www.github.com/bitcoin/bitcoin/pull/31132#discussion_r3146688138
- -> https://mirror.b10c.me/bitcoin-bitcoin/31132#discussion_r3146688138

Contributors

Labels

Linked (view graph)

#31132 validation: fetch block inputs on parallel threads

test: clean up threadpool test types #35174

Problem

Fix

Code Coverage & Benchmarks

Reviews