From da0b7f54177fcc666614f74ae4d7802affab8784 Mon Sep 17 00:00:00 2001 From: Chris Eidhof Date: Wed, 18 Jan 2017 12:36:28 +0100 Subject: [PATCH 01/10] Initial version --- proposals/XXXX-reduce-with-inout.md | 94 +++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 proposals/XXXX-reduce-with-inout.md diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/XXXX-reduce-with-inout.md new file mode 100644 index 0000000000..27ed84a8e5 --- /dev/null +++ b/proposals/XXXX-reduce-with-inout.md @@ -0,0 +1,94 @@ +# Feature name + +* Proposal: [SE-NNNN](NNNN-filename.md) +* Authors: [Chris Eidhof](https://github.com/chriseidhof) +* Review Manager: TBD +* Status: **Awaiting review** + +## Introduction + +A new variant of `reduce` should be added to the standard library. Instead of taking a `combine` function that is of type `(A, Iterator.Element) -> A`, the full type and implementation of the added `reduce` will be: + +```swift +extension Sequence { + func reduce(_ initial: A, combine: (inout A, Iterator.Element) -> ()) -> A { + var result = initial + for element in self { + combine(&result, element) + } + return result + } +} +``` + +Swift-evolution thread: [Reduce with inout](https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20170116/030300.html) + +## Motivation + +The current version of `reduce` needs to make copies of the result type for each element in the sequence. The proposed version can eliminate the need to make copies (when the `inout` is optimized away). + +## Proposed solution + +The first benefit of the proposed solution is efficiency. The new version of `reduce` can be used to write efficient implementations of methods that work on `Sequence`. For example, consider an implementation of `uniq`, which filters adjacent equal entries: + +```swift +extension Sequence where Iterator.Element: Equatable { + func uniq() -> [Iterator.Element] { + return reduce([]) { (result: inout [Iterator.Element], element) in + if result.last != element { + result.append(element) + } + } + } +} +``` + +In the code above, the optimizer will usually optimize the `inout` array into an `UnsafeMutablePointer`, or when inlined, into a simple mutable variable. With that optimization, the complexity of the algorithm is `O(n)`. The same algorithm, but implemented using the existing variant of reduce, will be `O(n²)`, because instead of `append`, it copies the existing array in the expression `result + [element]`: + +```swift +extension Sequence where Iterator.Element: Equatable { + func uniq() -> [Iterator.Element] { + return reduce([]) { (result: [Iterator.Element], element) in + guard result.last != element else { return result } + return result + [element] + } + } +} +``` + +The second benefit is that the new version of `reduce` is more natural when dealing with `mutating` methods. For example, consider a function that computes frequencies in a `Sequence`: + +```swift +extension Sequence where Iterator.Element: Hashable { + func frequencies() -> [Iterator.Element: Int] { + return reduce([:]) { (result: inout [Iterator.Element:Int], element) in + if let value = result[element] { + result[element] = value + 1 + } else { + result[element] = 1 + } + } + } +} +``` + +With the `inout` parameter, we'd first have to make a `var` copy of the existing result, and have to remember to return that copy instead of the `result`. (The method above is probably clearer when written with a `for`-loop, but that's not the point). + + +## Source compatibility + +This is purely additive, we don't propose removing the existing `reduce`. + +## Effect on ABI stability + +N/A + +## Effect on API resilience + +N/A + +## Alternatives considered + +We considered removing the existing `reduce`, but the problem with that is two-fold. First, it breaks existing code. Second, it's useful for algorithms that don't use mutating methods within `combine`. + +Under active discussion: the naming of this method. See the [swift-evolution thread](https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20170116/030300). From 11eb83e0fa6d5c30defc9a27bb08e42aea1ead09 Mon Sep 17 00:00:00 2001 From: Chris Eidhof Date: Wed, 18 Jan 2017 12:39:07 +0100 Subject: [PATCH 02/10] Typo --- proposals/XXXX-reduce-with-inout.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/XXXX-reduce-with-inout.md index 27ed84a8e5..8594733bdc 100644 --- a/proposals/XXXX-reduce-with-inout.md +++ b/proposals/XXXX-reduce-with-inout.md @@ -72,7 +72,7 @@ extension Sequence where Iterator.Element: Hashable { } ``` -With the `inout` parameter, we'd first have to make a `var` copy of the existing result, and have to remember to return that copy instead of the `result`. (The method above is probably clearer when written with a `for`-loop, but that's not the point). +Without the `inout` parameter, we'd first have to make a `var` copy of the existing result, and have to remember to return that copy instead of the `result`. (The method above is probably clearer when written with a `for`-loop, but that's not the point). ## Source compatibility From 06e8c7126f931a240c10dc24a23f7cc4c6349305 Mon Sep 17 00:00:00 2001 From: Chris Eidhof Date: Wed, 18 Jan 2017 12:40:14 +0100 Subject: [PATCH 03/10] Wording --- proposals/XXXX-reduce-with-inout.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/XXXX-reduce-with-inout.md index 8594733bdc..0a05ecc95a 100644 --- a/proposals/XXXX-reduce-with-inout.md +++ b/proposals/XXXX-reduce-with-inout.md @@ -89,6 +89,6 @@ N/A ## Alternatives considered -We considered removing the existing `reduce`, but the problem with that is two-fold. First, it breaks existing code. Second, it's useful for algorithms that don't use mutating methods within `combine`. +We considered removing the existing `reduce`, but the problem with that is two-fold. First, removing it breaks existing code. Second, it's useful for algorithms that don't use mutating methods within `combine`. Under active discussion: the naming of this method. See the [swift-evolution thread](https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20170116/030300). From 43dd5f3fdfa01c981f709e580178ae32ecd33746 Mon Sep 17 00:00:00 2001 From: Chris Eidhof Date: Thu, 19 Jan 2017 08:45:15 +0100 Subject: [PATCH 04/10] Title --- proposals/XXXX-reduce-with-inout.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/XXXX-reduce-with-inout.md index 0a05ecc95a..430f63199a 100644 --- a/proposals/XXXX-reduce-with-inout.md +++ b/proposals/XXXX-reduce-with-inout.md @@ -1,4 +1,4 @@ -# Feature name +# Reduce with `inout` * Proposal: [SE-NNNN](NNNN-filename.md) * Authors: [Chris Eidhof](https://github.com/chriseidhof) From 29959d07c23d246491ff67c39ceb80f0d4d52f68 Mon Sep 17 00:00:00 2001 From: Chris Eidhof Date: Tue, 24 Jan 2017 08:51:05 +0100 Subject: [PATCH 05/10] Tweaks --- proposals/XXXX-reduce-with-inout.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/XXXX-reduce-with-inout.md index 430f63199a..56d0b984e5 100644 --- a/proposals/XXXX-reduce-with-inout.md +++ b/proposals/XXXX-reduce-with-inout.md @@ -1,6 +1,6 @@ # Reduce with `inout` -* Proposal: [SE-NNNN](NNNN-filename.md) +* Proposal: [SE-NNNN](NNNN-reduce-with-inout.md) * Authors: [Chris Eidhof](https://github.com/chriseidhof) * Review Manager: TBD * Status: **Awaiting review** @@ -11,7 +11,7 @@ A new variant of `reduce` should be added to the standard library. Instead of ta ```swift extension Sequence { - func reduce(_ initial: A, combine: (inout A, Iterator.Element) -> ()) -> A { + func reduce(mutating: A, combine: (inout A, Iterator.Element) -> ()) -> A { var result = initial for element in self { combine(&result, element) @@ -34,7 +34,7 @@ The first benefit of the proposed solution is efficiency. The new version of `re ```swift extension Sequence where Iterator.Element: Equatable { func uniq() -> [Iterator.Element] { - return reduce([]) { (result: inout [Iterator.Element], element) in + return reduce(mutating: []) { (result: inout [Iterator.Element], element) in if result.last != element { result.append(element) } @@ -61,7 +61,7 @@ The second benefit is that the new version of `reduce` is more natural when deal ```swift extension Sequence where Iterator.Element: Hashable { func frequencies() -> [Iterator.Element: Int] { - return reduce([:]) { (result: inout [Iterator.Element:Int], element) in + return reduce(mutating: [:]) { (result: inout [Iterator.Element:Int], element) in if let value = result[element] { result[element] = value + 1 } else { @@ -77,7 +77,7 @@ Without the `inout` parameter, we'd first have to make a `var` copy of the exist ## Source compatibility -This is purely additive, we don't propose removing the existing `reduce`. +This is purely additive, we don't propose removing the existing `reduce`. Additionaly, because the first argument will have a label `mutating`, it doesn't add any extra burden to the type checker. ## Effect on ABI stability From ca0023502939b50d9a1f963a3f8f1ee8d8da1d84 Mon Sep 17 00:00:00 2001 From: Chris Eidhof Date: Tue, 24 Jan 2017 08:53:32 +0100 Subject: [PATCH 06/10] Make combine unnamed --- proposals/XXXX-reduce-with-inout.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/XXXX-reduce-with-inout.md index 56d0b984e5..152171710d 100644 --- a/proposals/XXXX-reduce-with-inout.md +++ b/proposals/XXXX-reduce-with-inout.md @@ -11,7 +11,7 @@ A new variant of `reduce` should be added to the standard library. Instead of ta ```swift extension Sequence { - func reduce(mutating: A, combine: (inout A, Iterator.Element) -> ()) -> A { + func reduce(mutating: A, _ combine: (inout A, Iterator.Element) -> ()) -> A { var result = initial for element in self { combine(&result, element) From 48b362cfe1abec0406bc0b0b8b40b0b50bc37965 Mon Sep 17 00:00:00 2001 From: Chris Eidhof Date: Tue, 24 Jan 2017 09:06:29 +0100 Subject: [PATCH 07/10] alternatives considered --- proposals/XXXX-reduce-with-inout.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/XXXX-reduce-with-inout.md index 152171710d..f0d3f3ca4a 100644 --- a/proposals/XXXX-reduce-with-inout.md +++ b/proposals/XXXX-reduce-with-inout.md @@ -89,6 +89,6 @@ N/A ## Alternatives considered -We considered removing the existing `reduce`, but the problem with that is two-fold. First, removing it breaks existing code. Second, it's useful for algorithms that don't use mutating methods within `combine`. +We considered removing the existing `reduce`, but the problem with that is two-fold. First, removing it breaks existing code. Second, it's useful for algorithms that don't use mutating methods within `combine`. We considered overloading `reduce`, but that would stress the type checker too much. Under active discussion: the naming of this method. See the [swift-evolution thread](https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20170116/030300). From 2b2a1d8cbadb38e9b520deafd8e6655f6b0a2d8b Mon Sep 17 00:00:00 2001 From: Chris Eidhof Date: Tue, 24 Jan 2017 09:33:40 +0100 Subject: [PATCH 08/10] Bug fix --- proposals/XXXX-reduce-with-inout.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/XXXX-reduce-with-inout.md index f0d3f3ca4a..f0765c7dde 100644 --- a/proposals/XXXX-reduce-with-inout.md +++ b/proposals/XXXX-reduce-with-inout.md @@ -11,7 +11,7 @@ A new variant of `reduce` should be added to the standard library. Instead of ta ```swift extension Sequence { - func reduce(mutating: A, _ combine: (inout A, Iterator.Element) -> ()) -> A { + func reduce(mutating initial: A, _ combine: (inout A, Iterator.Element) -> ()) -> A { var result = initial for element in self { combine(&result, element) From aa6c6c77441d69aa45cabb1a3db18fdb2373396f Mon Sep 17 00:00:00 2001 From: Chris Eidhof Date: Wed, 25 Jan 2017 10:09:09 +0100 Subject: [PATCH 09/10] Rename it into into --- proposals/XXXX-reduce-with-inout.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/XXXX-reduce-with-inout.md index f0765c7dde..bd440bccd8 100644 --- a/proposals/XXXX-reduce-with-inout.md +++ b/proposals/XXXX-reduce-with-inout.md @@ -11,7 +11,7 @@ A new variant of `reduce` should be added to the standard library. Instead of ta ```swift extension Sequence { - func reduce(mutating initial: A, _ combine: (inout A, Iterator.Element) -> ()) -> A { + func reduce(into initial: A, _ combine: (inout A, Iterator.Element) -> ()) -> A { var result = initial for element in self { combine(&result, element) @@ -34,7 +34,7 @@ The first benefit of the proposed solution is efficiency. The new version of `re ```swift extension Sequence where Iterator.Element: Equatable { func uniq() -> [Iterator.Element] { - return reduce(mutating: []) { (result: inout [Iterator.Element], element) in + return reduce(into: []) { (result: inout [Iterator.Element], element) in if result.last != element { result.append(element) } @@ -61,7 +61,7 @@ The second benefit is that the new version of `reduce` is more natural when deal ```swift extension Sequence where Iterator.Element: Hashable { func frequencies() -> [Iterator.Element: Int] { - return reduce(mutating: [:]) { (result: inout [Iterator.Element:Int], element) in + return reduce(into: [:]) { (result: inout [Iterator.Element:Int], element) in if let value = result[element] { result[element] = value + 1 } else { @@ -77,7 +77,7 @@ Without the `inout` parameter, we'd first have to make a `var` copy of the exist ## Source compatibility -This is purely additive, we don't propose removing the existing `reduce`. Additionaly, because the first argument will have a label `mutating`, it doesn't add any extra burden to the type checker. +This is purely additive, we don't propose removing the existing `reduce`. Additionaly, because the first argument will have a label `into`, it doesn't add any extra burden to the type checker. ## Effect on ABI stability @@ -91,4 +91,6 @@ N/A We considered removing the existing `reduce`, but the problem with that is two-fold. First, removing it breaks existing code. Second, it's useful for algorithms that don't use mutating methods within `combine`. We considered overloading `reduce`, but that would stress the type checker too much. +There has been a really active discussion about the naming of the first parameter. Naming it `mutating:` could deceive people into thinking that the value would get mutated in place. Naming it `mutatingCopyOf:` is also tricky: even though a copy of the struct gets mutated, copying is always implicit when using structs, and it wouldn't copy an instance of a class. `into:` seems the best name so far. + Under active discussion: the naming of this method. See the [swift-evolution thread](https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20170116/030300). From aa02506aa458319ea365d28b252c75d18f21edba Mon Sep 17 00:00:00 2001 From: Ben Cohen Date: Fri, 14 Apr 2017 11:34:33 -0700 Subject: [PATCH 10/10] Prepping for review --- ...{XXXX-reduce-with-inout.md => 0171-reduce-with-inout.md} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename proposals/{XXXX-reduce-with-inout.md => 0171-reduce-with-inout.md} (96%) diff --git a/proposals/XXXX-reduce-with-inout.md b/proposals/0171-reduce-with-inout.md similarity index 96% rename from proposals/XXXX-reduce-with-inout.md rename to proposals/0171-reduce-with-inout.md index bd440bccd8..5ce6c4134a 100644 --- a/proposals/XXXX-reduce-with-inout.md +++ b/proposals/0171-reduce-with-inout.md @@ -1,9 +1,9 @@ # Reduce with `inout` -* Proposal: [SE-NNNN](NNNN-reduce-with-inout.md) +* Proposal: [SE-0171](NNNN-reduce-with-inout.md) * Authors: [Chris Eidhof](https://github.com/chriseidhof) -* Review Manager: TBD -* Status: **Awaiting review** +* Review Manager: [Ben Cohen](https://github.com/airspeedswift) +* Status: **Active review (April 14...April 21, 2017)** ## Introduction