Skip to content

[OrderedCollections] Share OrderedSet's replace primitive with OrderedDictionary.replaceElement - #688

Open
inju2403 wants to merge 2 commits into
apple:mainfrom
inju2403:cleanup/replaceelement-reuse-replace
Open

[OrderedCollections] Share OrderedSet's replace primitive with OrderedDictionary.replaceElement#688
inju2403 wants to merge 2 commits into
apple:mainfrom
inju2403:cleanup/replaceelement-reuse-replace

Conversation

@inju2403

Copy link
Copy Markdown
Contributor

Summary

OrderedDictionary.replaceElement(at:withKey:value:) and OrderedSet.replace(at:with:) each performed the same "append the new member, swap it into position, then remove the old one from the end" step inline. This factors that step into an internal OrderedSet._replaceNew(at:with:in:) primitive shared by both. Behavior-preserving cleanup; no public API change.

Motivation

This is the follow-up flagged in #669. Adding OrderedSet.replace(at:with:) there, I noted that OrderedDictionary.replaceElement (added in #616) already performed this very operation by hand on its keys — _keys._appendNew, _keys.swapAt, _keys.removeLast — and could be reimplemented on top of replace. #669 left that out to keep it purely additive; this is that cleanup.

Rather than having replaceElement call replace(at:with:) directly, it shares only the mechanical step through an internal primitive. Calling replace directly is simpler, but it would route the dictionary's key mutation through OrderedSet's checks and replace the dictionary's own diagnostics:

Direct replace call Shared _replaceNew primitive (this PR)
Duplicate key traps with Duplicate element keeps Duplicate key: '\(key)'
Out-of-range index traps with Index out of bounds keeps Index out of range
Hash lookup re-runs _find inside replace reuses the bucket the caller already resolved

The primitive mirrors the existing _appendNew / _removeExistingMember internal primitives that OrderedDictionary already delegates its key-side mutations to.

Detailed design

Adds an internal OrderedSet._replaceNew(at:with:in:):

@inlinable
@discardableResult
internal mutating func _replaceNew(
  at index: Int, with item: Element, in bucket: _Bucket
) -> Element
  • Takes the target bucket already resolved by the caller's _find, appends item, swaps it into index, then removes the old member from the end — each step O(1). It performs no duplicate/bounds checks; the caller is responsible for those, matching _appendNew(_:in:) and _removeExistingMember(at:in:).
  • OrderedSet.replace(at:with:) now calls it after its own _find + precondition, instead of inlining the append/swap/remove.
  • OrderedDictionary.replaceElement's general case routes the key through _keys._replaceNew and overwrites the value at index in place — rather than moving the value through the same append/swap/remove step. Its equal-key in-place path and its own preconditions (Index out of range, Duplicate key: '\(key)') are unchanged.
  • No public API is added, changed, or removed; behavior and complexity (amortized O(1)) are unchanged.

Testing

Behavior-preserving refactor, so no new tests are added. The existing OrderedCollectionsTests suite passes, and the affected replace(at:with:) / replaceElement tests — which exercise every index across small element counts with both shared and unshared storage — pass with and without -Xswiftc -DCOLLECTIONS_INTERNAL_CHECKS.

Checklist

  • I've read the Contribution Guidelines
  • My contributions are licensed under the Swift license.
  • I've followed the coding style of the rest of the project.
  • I've added tests covering all new code paths my change adds to the project (if appropriate).
  • I've added benchmarks covering new functionality (if appropriate).
  • I've verified that my change does not break any existing tests or introduce unexplained benchmark regressions.
  • I've updated the documentation if necessary.

…dDictionary.replaceElement

apple#669 left `OrderedDictionary.replaceElement` untouched to keep that PR purely additive, noting it could later be reimplemented on top of `replace(at:with:)`. Both it and `OrderedSet.replace(at:with:)` performed the same "append the new member, swap it into position, then remove the old one from the end" step inline.

Factor that step into an internal `OrderedSet._replaceNew(at:with:in:)` primitive and call it from both, matching the existing `_appendNew` / `_removeExistingMember` primitives that `OrderedDictionary` already delegates its key-side mutations to. In `replaceElement` only the keys go through the primitive; the value at `index` is overwritten in place.

Sharing the mechanical step this way — rather than having `replaceElement` call `replace` directly — keeps each type's diagnostics intact: `OrderedDictionary` still traps with "Duplicate key: '\(key)'" and "Index out of range", not `OrderedSet`'s "Duplicate element" / "Index out of bounds". The shared step is already verified by the caller, so there is no behavior change.
@inju2403
inju2403 requested a review from lorentey as a code owner July 18, 2026 10:20

@lorentey lorentey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! One small nit below

…alue

Co-authored-by: Karoy Lorentey <klorentey@apple.com>
@lorentey lorentey added this to the 1.7.0 milestone Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants