refactor: unify container presence checks (without PR conflicts)

l0rinc commented at 2:29 am on August 15, 2025: contributor

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 have been changed, but we’ve reverted most of those to reduce conflict with other open PRs.

0rm -rfd build && \
1cmake -B build \
2  -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
3  -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
4  -DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
5  -DCMAKE_C_FLAGS="-target arm64-apple-macos11" \
6  -DCMAKE_CXX_FLAGS="-target arm64-apple-macos11" \
7  -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON
8
9 "$(brew --prefix llvm)/bin/run-clang-tidy" -quiet -p build -j$(nproc) -checks='-*,readability-container-contains' | grep -v 'clang-tidy'

Note: this is a take 2 of #33094 with fewer contentious changes.

refactor: unify container presence checks - find

The changes made here were:

| From                   | To               |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)`  |

2c590b4f55

refactor: unify container presence checks - trivial counts

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>

22c951f87c

refactor: unify container presence checks - non-trivial counts

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>

f70d2c7faa

DrahtBot added the label Refactoring on Aug 15, 2025

DrahtBot commented at 2:29 am on August 15, 2025: 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/33192.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	janb84

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:

#33230 (cli: Handle arguments that can be either JSON or string by achow101)
#31860 (init: Take lock on blocks directory in BlockManager ctor by TheCharlatan)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

janb84 commented at 8:53 am on August 15, 2025: contributor

ACK f70d2c7faa8f7d724e146e4c409de9c6778b7299

Hope this PR is now seen as valuable enough, still stand behind my last ACK reasons.

Q: Am I correct to conclude that the omission of the clang-tidy change is because of the limited scope of this PR? Any idea how we could work to get that rule included e.g. by some Linter ?

Old but still valid reasons to ACK this PR. 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.