refactor: unify container presence checks #33094

pull l0rinc wants to merge 3 commits into bitcoin:master from l0rinc:l0rinc/count-vs-contains changing 54 files +162 −162
  1. l0rinc commented at 11:38 pm on July 29, 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 be 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'
    
  2. DrahtBot added the label Refactoring on Jul 29, 2025
  3. DrahtBot commented at 11:38 pm on July 29, 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/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 (#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

  4. l0rinc marked this as ready for review on Jul 30, 2025
  5. in src/node/miner.cpp:340 in 35e27194ec outdated
    336@@ -337,7 +337,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
    337         if (mi != mempool.mapTx.get<ancestor_score>().end()) {
    338             auto it = mempool.mapTx.project<0>(mi);
    339             assert(it != mempool.mapTx.end());
    340-            if (mapModifiedTx.count(it) || inBlock.count(it->GetSharedTx()->GetHash()) || failedTx.count(it->GetSharedTx()->GetHash())) {
    341+            if (mapModifiedTx.count(it) || inBlock.contains(it->GetSharedTx()->GetHash()) || failedTx.contains(it->GetSharedTx()->GetHash())) {
    


    pablomartin4btc commented at 3:06 am on July 31, 2025:

    nit: not this one?

    0            if (mapModifiedTx.contains(it) || inBlock.contains(it->GetSharedTx()->GetHash()) || failedTx.contains(it->GetSharedTx()->GetHash())) {
    

    l0rinc commented at 4:02 am on July 31, 2025:
    I had these at one point, but decided against modifying multi-containers - but this one seems fine, thanks, added.
  6. pablomartin4btc commented at 3:19 am on July 31, 2025: member

    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.

  7. l0rinc force-pushed on Jul 31, 2025
  8. l0rinc commented at 4:01 am on July 31, 2025: contributor
    Thanks, split up the PR into 3 commits to simplify assessing the risks - hope this clarifies it, see the commit messages for further details.
  9. janb84 commented at 11:42 am on August 1, 2025: contributor

    Nit; Missed?: bitcoin-cli.cpp L11117

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

    =>

    0if (!proxy_networks.contains(proxy)) ordered_proxies.push_back(proxy);
    
  10. DrahtBot added the label Needs rebase on Aug 1, 2025
  11. l0rinc force-pushed on Aug 1, 2025
  12. l0rinc commented at 7:56 pm on August 1, 2025: contributor
    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
  13. DrahtBot removed the label Needs rebase on Aug 1, 2025
  14. janb84 commented at 9:13 am on August 4, 2025: contributor

    ACK 53461dc5cedf5f7080c09246ef76ba8a6c38a103

    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)

  15. DrahtBot requested review from pablomartin4btc on Aug 4, 2025
  16. fanquake commented at 10:41 am on August 5, 2025: member

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

     0[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
     1775 warnings generated.
     2
     3[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
     4/ci_container_base/src/test/coins_tests.cpp:397:48: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
     5  397 |                         assert(duplicate_coins.count(utxod->first));
     6      |                                                ^
     71128 warnings generated.
     8
     9[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
    10--
    11
    12[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
    1333 warnings generated.
    14
    15[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
    16/ci_container_base/src/wallet/test/fuzz/scriptpubkeyman.cpp:129:60: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
    17  129 |                     assert(spk_manager->GetScriptPubKeys().count(script));
    18      |                                                            ^
    192416 warnings generated.
    20
    21[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
    22--
    23
    24[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
    251071 warnings generated.
    26
    27[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
    28/ci_container_base/src/qt/walletframe.cpp:73:24: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
    29   73 |     if (mapWalletViews.count(walletView->getWalletModel()) > 0) return false;
    30      |                        ^~~~~                               ~~~
    31      |                        contains
    32/ci_container_base/src/qt/walletframe.cpp:93:24: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
    33   93 |     if (mapWalletViews.count(wallet_model) == 0) return;
    34      |                        ^~~~~               ~~~~
    35      |         !              contains
    36/ci_container_base/src/qt/walletframe.cpp:119:24: error: use 'contains' to check for membership [readability-container-contains,-warnings-as-errors]
    37  119 |     if (mapWalletViews.count(wallet_model) == 0) return;
    38      |                        ^~~~~               ~~~~
    39      |         !              contains
    401370 warnings generated.
    41
    42^^^ ⚠️ Failure generated from clang-tidy
    
  17. l0rinc force-pushed on Aug 5, 2025
  18. l0rinc commented at 6:47 pm on August 5, 2025: contributor
    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.
  19. DrahtBot added the label CI failed on Aug 5, 2025
  20. DrahtBot commented at 8:26 pm on August 5, 2025: contributor

    🚧 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.

    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.

  21. l0rinc force-pushed on Aug 5, 2025
  22. DrahtBot removed the label CI failed on Aug 5, 2025
  23. janb84 commented at 3:10 pm on August 6, 2025: contributor

    re ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb

    changes since last ACK:

    • clang-tidy related changes
    0$ ( cd ./src/ && run-clang-tidy -p ../build -j $(nproc) ) | grep walletframe.cpp
    1<< no warnings>> 
    
  24. optout21 commented at 7:16 pm on August 6, 2025: none

    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.

  25. l0rinc commented at 7:36 pm on August 6, 2025: contributor
    Yes, applies to any kind of map, but I had to find and change those manually - clang-tidy doesn’t detect or fix those.
  26. optout21 commented at 7:40 pm on August 6, 2025: none

    ACK 05153600af10c322e40d107b8cf0c8e69b8b0aeb

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

  27. DrahtBot added the label Needs rebase on Aug 13, 2025
  28. l0rinc commented at 11:08 pm on August 13, 2025: contributor

    Rebased after #33116, ready for review again:

    git range-diff 05153600af10c322e40d107b8cf0c8e69b8b0aeb…995d3a9f11e06a518c39a5042576e812628aa3d6

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

  29. l0rinc force-pushed on Aug 13, 2025
  30. ajtowns commented at 0:58 am on August 14, 2025: contributor
    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?
  31. DrahtBot removed the label Needs rebase on Aug 14, 2025
  32. l0rinc commented at 1:18 am on August 14, 2025: contributor
    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.
  33. ajtowns commented at 3:01 am on August 14, 2025: contributor

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

    1. 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)
    2. Run clang-tidy
    3. Check for any clang-tidy errors that hit lines found in step 1, and error out if there are any hits
  34. 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
  35. 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
  36. 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
  37. l0rinc force-pushed on Aug 15, 2025
  38. l0rinc closed this on Aug 15, 2025

  39. l0rinc reopened this on Aug 15, 2025

  40. l0rinc closed this on Aug 15, 2025

  41. l0rinc commented at 2:31 am on August 15, 2025: contributor
    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
  42. maflcko commented at 11:56 am on August 18, 2025: member

    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. 54d0022e5d6921a31f5a2fc9302f4c4fc636b6a8), …)

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-24 09:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me