refactor(tidy): Use C++20 contains method #29191

aureleoules commented at 5:22 PM on January 5, 2024: contributor

C++20 introduced the contains method on containers to check if an element is in the container. It can be used instead of the count method now. I believe it is easier to understand, as contains directly returns a bool indicating whether the element exists, while count returns the number of occurrences of the element, which then often needs to be compared against 0. Also, it is slightly more efficient than count for this use-case.

This pull request introduces the clang-tidy check readability-container-contains and fixes the instances where count is being used where contains could be used instead. I used the clang-tidy -fix option to do it automatically.

DrahtBot commented at 5:22 PM on January 5, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theuni, TheCharlatan, theStack, BrandonOdiwuor

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:

#29236 (log: Nuke error(...) by maflcko)
#28948 (v3 transaction policy for anti-pinning by glozow)
#28886 (refactor: Replace sets of txiter with CTxMemPoolEntryRefs by TheCharlatan)
#28806 (rpc: Add script verification flags to getdeploymentinfo by ajtowns)
#28560 (wallet, rpc: FundTransaction refactor by josibake)
#28201 (Silent Payments: sending by josibake)
#28170 (p2p: adaptive connections services flags by furszy)
#28031 (Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling by glozow)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#27101 (Support JSON-RPC 2.0 when requested by client by pinheadmz)
#26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
#26114 (net: Make AddrFetch connections to fixed seeds by mzumsande)
#25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)

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.

theuni commented at 5:28 PM on January 5, 2024: member

Nice use of tidy!

Concept ACK, assuming ::contains() exists for common stdlibs by now. Waiting to see what c-i thinks.

TheCharlatan commented at 5:41 PM on January 5, 2024: contributor

Concept ACK

aureleoules force-pushed on Jan 5, 2024

DrahtBot added the label CI failed on Jan 5, 2024

DrahtBot removed the label CI failed on Jan 5, 2024

theStack commented at 1:35 PM on January 7, 2024: contributor

Nice! Concept ACK

BrandonOdiwuor commented at 7:06 AM on January 8, 2024: contributor

Concept ACK using ::contains(Key& key) (C++20) to check for elements

TheCharlatan commented at 11:38 AM on January 8, 2024: contributor

Can the full diff be reproduced with run-clang-tidy -fix? If so, can you post the full commands you are using? I am getting a slightly different diff if I run it myself.

aureleoules commented at 11:42 AM on January 8, 2024: contributor

Can the full diff be reproduced with run-clang-tidy -fix? If so, can you post the full commands you are using? I am getting a slightly different diff if I run it myself.

Sorry, my initial push did not pass the tidy check because it seems the -fix option does not fix code in macros (all the asserts, Assume, etc.) so I manually fixed these in a second push. This may be the reason why you are not getting the same diff as me.

I don't have the specific command I used on hand but I used the find command to retrieve all cpp and h files, piped to clang-tidy-17 -fix if I remember correctly.

TheCharlatan commented at 5:33 PM on January 8, 2024: contributor

Re #29191 (comment)

Sorry, my initial push did not pass the tidy check because it seems the -fix option does not fix code in macros (all the asserts, Assume, etc.) so I manually fixed these in a second push. This may be the reason why you are not getting the same diff as me.

I see, that is a bit unfortunate. I also found some more things to change: https://github.com/TheCharlatan/bitcoin/commit/63fcfbecd133551032857187e5e639d36a2f7b06, but not sure if non-tidy changes should be made here tbh.

aureleoules force-pushed on Jan 8, 2024

aureleoules commented at 6:39 PM on January 8, 2024: contributor

I see, that is a bit unfortunate. I also found some more things to change: https://github.com/TheCharlatan/bitcoin/commit/63fcfbecd133551032857187e5e639d36a2f7b06, but not sure if non-tidy changes should be made here tbh.

Thanks for the commit, I added it. Did you find them manually or with clang-tidy? I'm not sure why the CI didn't catch them...

refactor(tidy): use c++20 contains 90c4d91bf2

in src/addrman.cpp:912 in 520669e627 outdated

 908 | @@ -909,7 +909,7 @@ void AddrManImpl::ResolveCollisions_()
 909 |          bool erase_collision = false;
 910 |  
 911 |          // If id_new not found in mapInfo remove it from m_tried_collisions
 912 | -        if (mapInfo.count(id_new) != 1) {
 913 | +        if (!mapInfo.count(id_new)) {

mzumsande commented at 6:54 PM on January 8, 2024:

count -> contains

aureleoules commented at 7:00 PM on January 8, 2024:

Thanks, fixed.

aureleoules force-pushed on Jan 8, 2024

stickies-v commented at 12:19 PM on January 9, 2024: contributor

I support using contains going forward, but I'm not sure for this PR the benefits outweigh the costs?

I prefer the contains interface but imo using count is also pretty straightforward, so the benefits seem limited?
It conflicts with a large number of PRs
It's not a scripted diff

I think I'd prefer removing the clang-tidy check for now and only changing it in places where it doesn't conflict with other PRs, e.g. maybe in tests? And then organically have more contains adoption over time?

TheCharlatan commented at 5:10 PM on January 14, 2024: contributor

Re #29191 (comment)

but I'm not sure for this PR the benefits outweigh the costs?

FWIW I would approve this PR if it would only contain the changes that are automatically picked up by tidy and applied by running with -fix, making resolving potential conflicts purely manual.

DrahtBot added the label CI failed on Jan 15, 2024

aureleoules commented at 10:19 PM on January 16, 2024: contributor

FWIW I would approve this PR if it would only contain the changes that are automatically picked up by tidy and applied by running with -fix, making resolving potential conflicts purely manual.

Sure, I agree. Closing this pull.

aureleoules closed this on Jan 16, 2024

bitcoin locked this on Jan 15, 2025