-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Efficient Dictionary.mapValues with key context
#3060
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Efficient Dictionary.mapValues with key context
#3060
Conversation
b967289 to
a90d3e5
Compare
|
|
||
| ### Alternative naming | ||
|
|
||
| The original draft of this proposal planned on overloading the existing `mapValues` method to accept a closure that takes both `Key` and `Value`. This was discovered to be source-breaking in rare scenarios where `mapValues` was being called on a dictionary with a 2-tuple value type. Thus, the new name `mapValuesWithKeys` was chosen to avoid source compatibility issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be interesting to think about how we'd handle incremental migration to a world in which mapValues provides the key to the transform (which I think most of us is the more desirable design). It ought to be possible to do so by doing something like introducing dedicated naming for both operations, and then using a feature flag to toggle which one is bound by the mapValues name, and we could then set that feature flag in a future language version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a section at the end
|
|
||
| ### Adoption of typed `throws` for pre-existing methods | ||
|
|
||
| The proposed `mapValuesWithKeys` method uses typed `throws`, unlike the existing `mapValues` method which uses untyped `throws`. In the future, we may wish to introduce a typed `throws` variant of `mapValues` as well, and take the API break opportunity to select a new name for this method, which would then enable the standard library to eventually reassign the `mapValues` name to the version that supplies key context to the transformation closure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typed-throw adoption is not an API break (also, the PR to adopt typed throws for mapValues should land soon). So I think typed-throws adoption is unrelated.
Rather, we would do something like give the existing mapValues an explicit name like mapValuesWithoutKeys and then tie the meaning of mapValues to an upcoming language feature (upcoming library feature?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, i rewrote that paragraph
Efficient
Dictionary.mapValueswith key contextI propose adding an overload to
Dictionary.mapValues(andOrderedDictionary.mapValues) that passes theKeyto the transformation closure.This enables us to transform dictionary values with their associated key context without incurring the performance cost of rehashing (or in the case of
reduce, reallocating) the dictionary storage, which is currently unavoidable when usinginit(uniqueKeysWithValues:)orreduce(into:).