refactor: unify container presence checks #33094

l0rinc · 2025-07-29T23:38:27Z

Summary

Instead of counting occurrences in sets and maps, the C++20 ::contains method expresses the intent unambiguously and can return early on first encounter.

Context

Applied clang‑tidy's readability‑container‑contains check, though many cases required manual changes since tidy couldn't fix them automatically.

Changes

The changes made here were:

From	To
`m.find(k) == m.end()`	`!m.contains(k)`
`m.find(k) != m.end()`	`m.contains(k)`
`m.count(k)`	`m.contains(k)`
`!m.count(k)`	`!m.contains(k)`
`m.count(k) == 0`	`!m.contains(k)`
`m.count(k) != 1`	`!m.contains(k)`
`m.count(k) == 1`	`m.contains(k)`
`m.count(k) < 1`	`!m.contains(k)`
`m.count(k) > 0`	`m.contains(k)`
`m.count(k) != 0`	`m.contains(k)`

Note that == 1/!= 1/< 1 only apply to simple maps/sets and had to be changed manually.

There are many other cases that could be changed, but we've reverted most of those to reduce conflict with other open PRs.

clang-tidy command on Mac

rm -rfd build && \
cmake -B build \
  -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
  -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
  -DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
  -DCMAKE_C_FLAGS="-target arm64-apple-macos11" \
  -DCMAKE_CXX_FLAGS="-target arm64-apple-macos11" \
  -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON

 "$(brew --prefix llvm)/bin/run-clang-tidy" -quiet -p build -j$(nproc) -checks='-*,readability-container-contains' | grep -v 'clang-tidy'

DrahtBot · 2025-07-29T23:38:32Z

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/33094.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	pablomartin4btc
Stale ACK	janb84, optout21

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33062 (truc: optimize the in package relation calculation by HowHsu)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#33032 (wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase by achow101)
#33018 (coins: remove SetFresh method from CCoinsCacheEntry by andrewtoth)
#32998 (Bump SCRIPT_VERIFY flags to 64 bit by ajtowns)
#32763 (wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members by achow101)
#32523 (wallet: Remove isminetypes by achow101)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
#31860 (init: Take lock on blocks directory in BlockManager ctor by TheCharlatan)
#31713 (miniscript refactor: Remove unique_ptr-indirection (descriptor: Add proper Clone function to miniscript::Node #30866 follow-up) by hodlinator)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#26781 (Release LockData::dd_mutex before calling *_detected functions by hebasto)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

"a available transaction" → "an available transaction" [‘available’ begins with a vowel sound, so ‘an’ is required]

^{drahtbot_id_4_m}

src/node/miner.cpp

pablomartin4btc

Concept ACK

This refactor replaces explicit .count(x) checks with .contains(x), which only verifies that at least one instance exists (or none when == 0). Please double-check whether .contains() preserves the original behavior in some contexts where exactness matters ( !=1, ==1, etc). Also where the logic enforces that exactly one item is/ is not present — useful even in containers that don't allow duplicates (like std::map or std::set) because it clearly expresses the expected logic — replacing it with .contains() weakens the check and may mask issues where the assumption of uniqueness is violated.

l0rinc

Thanks, split up the PR into 3 commits to simplify assessing the risks - hope this clarifies it, see the commit messages for further details.

janb84 · 2025-08-01T11:42:12Z

Nit; Missed?:
bitcoin-cli.cpp L11117

if (proxy_networks.find(proxy) == proxy_networks.end()) ordered_proxies.push_back(proxy);

=>

if (!proxy_networks.contains(proxy)) ordered_proxies.push_back(proxy);

l0rinc · 2025-08-01T19:56:42Z

Thanks, rebased to solve conflict in validations.cpp and migrated a few new values - thanks for the hint @janb84.
You can re-review via git range-diff master 71911a59 53461dc5

janb84

ACK 53461dc

This PR refactors the code to use the more modern contains() method. In my opinion this PR increased the readability of the code and removes the ambiguity of the intention of the count() methods used. With this change, the intent to enforce that exactly one item is/ is not present or just a presence check will be more obvious. (count() vs contains()).

The argument NOT to change the code because of the risk losing the original behaviour is an indication to me that the refactor is needed to remove the ambiguity and more clearly communicate the intent of the code.

(thanks for including my suggestion/nit)

fanquake · 2025-08-05T10:41:20Z

if this is done, it should be added to clang-tidy:

[379/677][7.2s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/zmq/zmqrpc.cpp
775 warnings generated.

[380/677][10.0s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/test/coins_tests.cpp
/ci_container_base/src/test/coins_tests.cpp:397:48: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
  397 |                         assert(duplicate_coins.count(utxod->first));
      |                                                ^
1128 warnings generated.

[381/677][10.2s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/test/txrequest_tests.cpp
--

[443/677][0.8s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/crypto/sha3.cpp
33 warnings generated.

[444/677][16.6s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/wallet/test/fuzz/scriptpubkeyman.cpp
/ci_container_base/src/wallet/test/fuzz/scriptpubkeyman.cpp:129:60: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
  129 |                     assert(spk_manager->GetScriptPubKeys().count(script));
      |                                                            ^
2416 warnings generated.

[445/677][17.4s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/wallet/rpc/transactions.cpp
--

[653/677][12.1s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/test/fuzz/miniscript.cpp
1071 warnings generated.

[654/677][14.3s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/qt/walletframe.cpp
/ci_container_base/src/qt/walletframe.cpp:73:24: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
   73 |     if (mapWalletViews.count(walletView->getWalletModel()) > 0) return false;
      |                        ^~~~~                               ~~~
      |                        contains
/ci_container_base/src/qt/walletframe.cpp:93:24: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
   93 |     if (mapWalletViews.count(wallet_model) == 0) return;
      |                        ^~~~~               ~~~~
      |         !              contains
/ci_container_base/src/qt/walletframe.cpp:119:24: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
  119 |     if (mapWalletViews.count(wallet_model) == 0) return;
      |                        ^~~~~               ~~~~
      |         !              contains
1370 warnings generated.

^^^ ⚠️ Failure generated from clang-tidy

l0rinc · 2025-08-05T18:47:46Z

Thanks @fanquake, I wanted to leave out QMap values, but since it enables adding readability-container-contains to clang-tidy, I'm all for it.
Done in the latest push in the last commit (since it's a non-trivial map type) - where I also rebased to make sure we're capturing all potential recent additions.
Should be easy to re-review.

DrahtBot · 2025-08-05T20:26:54Z

🚧 At least one of the CI tasks failed.
_{Task tidy: https://github.com/bitcoin/bitcoin/runs/47447365178}
_{LLM reason (✨ experimental): Clang-tidy reported errors regarding usage of 'contains' for container membership checks.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

janb84

re ACK 0515360

changes since last ACK:

clang-tidy related changes

New result of clang-tidy :

$ ( cd ./src/ && run-clang-tidy -p ../build -j $(nproc) ) | grep walletframe.cpp
<< no warnings>>

optout21 · 2025-08-06T19:16:00Z

Note that == 1/!= 1/< 1 only apply to simple maps/sets and had to be changed manually.

I understand the == 1/!= 1 parts, but for < 1: it means that the value can be only 0, which means "doesn't contain", so it applies equally to de-duplicated containers (map, set) and duplicated containers as well, isn't it?

Update: AFAICS the < 1 pattern is changed in only one place, in a multi-map.

l0rinc · 2025-08-06T19:36:33Z

Yes, applies to any kind of map, but I had to find and change those manually - clang-tidy doesn't detect or fix those.

optout21

ACK 0515360

The changes increase code readability, as 'contains' expresses the code logic / intent more specifically
It also results in higher performance, due to potential early exit. The improvement is probably negligible though.
Changes are localized (each to a single line), local impact only

(I've reviewed the proposed changed, but didn't look for other potential changes)

l0rinc · 2025-08-13T23:08:54Z

Rebased after #33116, ready for review again:

git range-diff 0515360...995d3a9

I have added a few more count -> contains changes that were introduced since.

ajtowns · 2025-08-14T00:58:08Z

Concept -1: this doesn't seem valuable enough to warrant rebasing all the PRs it conflicts with. Can this be added as a linter of some sort to encourage the changes to be made when the code is touched?

l0rinc · 2025-08-14T01:18:03Z

Not sure what to do about the conflicts, but to enable the linter, I had to fix the existing problems first. Maybe there is a way in clang-tidy to only check new code, but it's definitely cleaner enabling it for all code, as done in this PR.
I also had to do many of these changes manually, so while linter would help, it cannot catch all of these inconsistencies.

ajtowns · 2025-08-14T03:01:59Z

Not sure what to do about the conflicts, but to enable the linter, I had to fix the existing problems first. Maybe there is a way in clang-tidy to only check new code,

Perhaps an approach like this would work (for this and other cleanups)?

Run git diff ${base}..HEAD --unified=0 and use the ^[+][+][+] lines to track the files touched, and the "+n,k" values in the ^@@ lines to identify the touched lines (ie, each line in range(n,n+k)
Run clang-tidy
Check for any clang-tidy errors that hit lines found in step 1, and error out if there are any hits

The changes made here were: | From | To | |------------------------|------------------| | `m.find(k) == m.end()` | `!m.contains(k)` | | `m.find(k) != m.end()` | `m.contains(k)` |

The changes made here were: | From | To | |-------------------|------------------| | `m.count(k)` | `m.contains(k)` | | `!m.count(k)` | `!m.contains(k)` | | `m.count(k) == 0` | `!m.contains(k)` | | `m.count(k) != 0` | `m.contains(k)` | | `m.count(k) > 0` | `m.contains(k)` | The commit contains the trivial, mechanical refactors where it doesn't matter if the container can have multiple elements or not Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>

The changes made here were: | From | To | |-------------------|------------------| | `m.count(k) == 1` | `m.contains(k)` | | `m.count(k) != 1` | `!m.contains(k)` | | `m.count(k) < 1` | `!m.contains(k)` | * `mapInfo` is instance of `std::unordered_map` and can only contain 0 or 1 value for a given key; * similarly, `g_enabled_filter_types` and `setClientRules` are both `std::set` instances; * lastly, while `mapTxSpends` is `std::unordered_multimap` that could potentially hold multiple values, having a size less than 1 means that the value is missing. `QMap<WalletModel*, WalletView*> mapWalletViews` values were also migrated manually. Co-authored-by: pablomartin4btc <pablomartin4btc@gmail.com> Co-authored-by: fanquake <fanquake@gmail.com>

l0rinc · 2025-08-15T02:31:52Z

Removed all conflicts with other PR that I found and opened a new PR at #33192 to give @DrahtBot a chance to recalculate the conflicts section.
cc: @ajtowns, @instagibbs

maflcko · 2025-08-18T11:56:11Z

It is already possible to run clang tidy on a diff. See git grep clang-tidy-diff. However, it is not integrated into CI. In theory it could be integrated, enforcing a stronger set of checks than the "default" ones. Though, that's also going to mess with move-only commits, so I am not sure if it will be useful to have it enabled and lead to failing CI. I'd say there are two options:

Enable it globally and by default in a "breaking" CI change that may lead to silent or explicit merge conflicts.
Just add it to one of the existing reporting channels for non-critical issues (corecheck (freshly added or as sonarcloud via corecheck), DrahtBot summary comment, CI notice/warning (c.f. 54d0022), ...)

DrahtBot added the Refactoring label Jul 29, 2025

l0rinc marked this pull request as ready for review July 30, 2025 05:54

pablomartin4btc reviewed Jul 31, 2025

View reviewed changes

src/node/miner.cpp Outdated Show resolved Hide resolved

pablomartin4btc reviewed Jul 31, 2025

View reviewed changes

l0rinc force-pushed the l0rinc/count-vs-contains branch from 35e2719 to 71911a5 Compare July 31, 2025 04:00

l0rinc commented Jul 31, 2025

View reviewed changes

DrahtBot added the Needs rebase label Aug 1, 2025

l0rinc force-pushed the l0rinc/count-vs-contains branch from 71911a5 to 53461dc Compare August 1, 2025 19:54

DrahtBot removed the Needs rebase label Aug 1, 2025

This was referenced Aug 2, 2025

refactor: Convert uint256 to Txid #33116

Merged

Bump SCRIPT_VERIFY flags to 64 bit #32998

Open

kernel, refactor: return error status on all fatal errors #29700

Draft

janb84 reviewed Aug 4, 2025

View reviewed changes

DrahtBot requested a review from pablomartin4btc August 4, 2025 09:14

l0rinc force-pushed the l0rinc/count-vs-contains branch from 53461dc to d397f0b Compare August 5, 2025 18:45

DrahtBot added the CI failed label Aug 5, 2025

l0rinc force-pushed the l0rinc/count-vs-contains branch from d397f0b to 0515360 Compare August 5, 2025 20:42

DrahtBot removed the CI failed label Aug 5, 2025

janb84 reviewed Aug 6, 2025

View reviewed changes

optout21 reviewed Aug 6, 2025

View reviewed changes

DrahtBot mentioned this pull request Aug 8, 2025

init: Take lock on blocks directory in BlockManager ctor #31860

Draft

DrahtBot added the Needs rebase label Aug 13, 2025

l0rinc force-pushed the l0rinc/count-vs-contains branch from 0515360 to 995d3a9 Compare August 13, 2025 23:08

DrahtBot removed the Needs rebase label Aug 14, 2025

l0rinc and others added 3 commits August 14, 2025 18:47

refactor: unify container presence checks - find

2c590b4

The changes made here were: | From | To | |------------------------|------------------| | `m.find(k) == m.end()` | `!m.contains(k)` | | `m.find(k) != m.end()` | `m.contains(k)` |

l0rinc force-pushed the l0rinc/count-vs-contains branch from 995d3a9 to f70d2c7 Compare August 15, 2025 01:51

l0rinc closed this Aug 15, 2025

l0rinc reopened this Aug 15, 2025

l0rinc closed this Aug 15, 2025

l0rinc mentioned this pull request Aug 15, 2025

refactor: unify container presence checks (without PR conflicts) #33192

Open

refactor: unify container presence checks #33094

refactor: unify container presence checks #33094

Uh oh!

Conversation

l0rinc commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Context

Changes

Uh oh!

DrahtBot commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

janb84 commented Aug 1, 2025

Uh oh!

l0rinc commented Aug 1, 2025

Uh oh!

janb84 left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 5, 2025

Uh oh!

l0rinc commented Aug 5, 2025

Uh oh!

DrahtBot commented Aug 5, 2025

Uh oh!

janb84 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optout21 commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l0rinc commented Aug 6, 2025

Uh oh!

optout21 left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Aug 13, 2025

Uh oh!

ajtowns commented Aug 14, 2025

Uh oh!

l0rinc commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Aug 14, 2025

Uh oh!

l0rinc commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 18, 2025

Uh oh!

Uh oh!

l0rinc commented Jul 29, 2025 •

edited

Loading

DrahtBot commented Jul 29, 2025 •

edited

Loading

janb84 left a comment •

edited

Loading

optout21 commented Aug 6, 2025 •

edited

Loading

l0rinc commented Aug 14, 2025 •

edited

Loading

l0rinc commented Aug 15, 2025 •

edited

Loading