refactor: unify container presence checks (without PR conflicts) #33192
Conversation
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>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33192. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
ACK f70d2c7
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.
- code review ✅
- build & tested ✅
Summary
Instead of counting occurrences in sets and maps, the C++20
::containsmethod 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:
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) == 1m.contains(k)m.count(k) < 1!m.contains(k)m.count(k) > 0m.contains(k)m.count(k) != 0m.contains(k)There are many other cases that could have been changed, but we've reverted most of those to reduce conflict with other open PRs.
clang-tidy command on Mac
Note: this is a take 2 of #33094 with fewer contentious changes.