refactor: unify container presence checks #33094

pull l0rinc wants to merge 3 commits into bitcoin:master from l0rinc:l0rinc/count-vs-contains changing 63 files +189 −189
  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.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)
    m.find(k) == m.end() !m.contains(k)
    m.find(k) != m.end() m.contains(k)

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


    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

    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:

    • #33116 (refactor: Convert uint256 to Txid by marcofleon)
    • #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)
    • #32844 (RPC/txoutproof: Support including (and verifying) proofs of wtxid by luke-jr)
    • #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)
    • #31713 (miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up) by hodlinator)
    • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #28676 (Cluster mempool implementation by sdaftuar)
    • #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.

  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. 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)`  |
    537a221a93
  12. 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>
    ff837a894b
  13. 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.
    
    Co-authored-by: pablomartin4btc <pablomartin4btc@gmail.com>
    53461dc5ce
  14. l0rinc force-pushed on Aug 1, 2025
  15. 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
  16. DrahtBot removed the label Needs rebase on Aug 1, 2025

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-04 00:12 UTC

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