Skip to content

[OrderedCollections] Use exchange(_:with:) in replaceElement's equal-key path - #703

Open
inju2403 wants to merge 1 commit into
apple:mainfrom
inju2403:cleanup/replaceelement-exchange-same-key
Open

[OrderedCollections] Use exchange(_:with:) in replaceElement's equal-key path#703
inju2403 wants to merge 1 commit into
apple:mainfrom
inju2403:cleanup/replaceelement-exchange-same-key

Conversation

@inju2403

Copy link
Copy Markdown
Contributor

Summary

OrderedDictionary.replaceElement(at:withKey:value:) overwrites the value at index in both of its paths. #688 switched one of them to exchange(_:with:); this switches the other, so both move the old value out instead of copying it. Behavior-preserving; no public API change.

Motivation

#688 factored the general case onto OrderedSet._replaceNew(at:with:in:), and in review adopted exchange(_:with:) for the value overwrite that the same commit had just rewritten. The equal-key path above it wasn't part of that diff — #688 noted it was left unchanged — so it still does the read-then-assign it has always done.

Going back over replaceElement and replace(at:with:) now that #688 has landed, this is the one place left in the function where the old value is copied out before being overwritten, rather than moved out. The two paths do the same thing to _values[index] a dozen lines apart; they should be written the same way:

 if existingIndex == index {
   let oldKey = _keys.update(key, at: index)
-  let oldValue = _values[index]
-  _values[index] = value
+  let oldValue = exchange(&_values[index], with: value)
   _checkInvariants()
   return (oldKey, oldValue)
 }

Nothing else moves: the key still goes through _keys.update(key, at: index), the hash table is still left untouched on this path, and replaceElement's preconditions, returned element, and complexity (expected amortized O(1)) are unchanged. No public API is added, changed, or removed.

Testing

Behavior-preserving change, so no new tests are added. This path is already covered by test_replaceElement_sameKey in both OrderedDictionary Tests.swift and OrderedDictionary+Elements Tests.swift — added alongside the equal-key path itself — covering sizes 1 through 19 with every valid index position, both unique and shared (copy-on-write) storage via withHiddenCopies, lifetime tracking, and the returned old key and value. All 261 OrderedCollectionsTests 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.

…key path

apple#688 factored `replaceElement`'s general case onto `OrderedSet._replaceNew(at:with:in:)` and, per review feedback, switched the value overwrite there to `exchange(_:with:)`. The equal-key path above it was outside that diff, so it still reads the old value into a local and then assigns over it.

Switch it to `exchange(_:with:)` as well, so both paths move the old value out instead of copying it. No behavior change: the returned element, the preconditions, and the expected amortized O(1) complexity are all unchanged.
@inju2403
inju2403 requested a review from lorentey as a code owner August 14, 2026 00:30

@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.

Thank you!

@lorentey lorentey added this to the 1.7.0 milestone Aug 18, 2026
@lorentey

Copy link
Copy Markdown
Member

The failing check is due to a CI issue; #706 will eventually resolve it.

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