[PR #3105] [MERGED] Use retain() instead of calling remove() in a loop #3560

New issue

Closed

opened 2026-03-16 11:50:26 +03:00 by kerem · 0 comments

kerem commented

2026-03-16 11:50:26 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hickory-dns/hickory-dns/pull/3105
Author: @divergentdave
Created: 7/9/2025
Status: ✅ Merged
Merged: 7/9/2025
Merged by: @djc

Base: main ← Head: david/retain-optimizations

📝 Commits (1)

0c61e26 Use retain() instead of calling remove() in a loop

📊 Changes

3 files changed (+22 additions, -46 deletions)

View changed files

📝 crates/proto/src/rr/rr_set.rs (+5 -12)
📝 crates/server/src/store/in_memory/inner.rs (+4 -20)
📝 crates/server/src/store/sqlite/mod.rs (+13 -14)

📄 Description

I noticed a pattern in a few places of iterating over a collection, filtering the iterator, collecting the indexes or keys into a vector, and then calling remove() for each index or key in the vector. This PR replaces this pattern with a call to retain() instead, using the logical inverse of the filter predicate. This should be both more idiomatic and more efficient.

Furthermore, note that RecordSet::remove() removes vector elements by index, in increasing index order. This would not be safe if it ever removed more than one element, because the first Vec::remove() call shifts the tail of the vector left, invalidating the semantics of the rest of the stored indices. In this case I think it was fine, because of the invariant that an RRset cannot contain any duplicate records, but just using retain() is less fragile.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/hickory-dns/hickory-dns/pull/3105 **Author:** [@divergentdave](https://github.com/divergentdave) **Created:** 7/9/2025 **Status:** ✅ Merged **Merged:** 7/9/2025 **Merged by:** [@djc](https://github.com/djc) **Base:** `main` ← **Head:** `david/retain-optimizations` --- ### 📝 Commits (1) - [`0c61e26`](https://github.com/hickory-dns/hickory-dns/commit/0c61e269b2e7cc65285619b220626e45cafdf90a) Use retain() instead of calling remove() in a loop ### 📊 Changes **3 files changed** (+22 additions, -46 deletions) <details> <summary>View changed files</summary> 📝 `crates/proto/src/rr/rr_set.rs` (+5 -12) 📝 `crates/server/src/store/in_memory/inner.rs` (+4 -20) 📝 `crates/server/src/store/sqlite/mod.rs` (+13 -14) </details> ### 📄 Description I noticed a pattern in a few places of iterating over a collection, filtering the iterator, collecting the indexes or keys into a vector, and then calling `remove()` for each index or key in the vector. This PR replaces this pattern with a call to `retain()` instead, using the logical inverse of the filter predicate. This should be both more idiomatic and more efficient. Furthermore, note that `RecordSet::remove()` removes vector elements by index, in increasing index order. This would not be safe if it ever removed more than one element, because the first `Vec::remove()` call shifts the tail of the vector left, invalidating the semantics of the rest of the stored indices. In this case I think it was fine, because of the invariant that an RRset cannot contain any duplicate records, but just using `retain()` is less fragile. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>