SE-0173: Add `MutableCollection.swap(_:with:)


(Ted Kremenek) #1

Hello Swift community,

The review of SE-0173 "Add MutableCollection.swap(_:with:)" begins now and runs through April 28, 2017.

The proposal is available here:


Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at:

https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

Reply text

Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:


Thank you,
Ted (Review Manager)


(Saagar Jha) #2

+1 overall; the proposal is well justified and I don’t see any reason why it should be rejected.

One nitpick I do have is with the choice of the argument parameter: elements.swap(i, with: j) seems at first glance (to me, at least) to swap the elements i and j, not the element at i with the element at j. The original (free) function made sense because it actually performed its operation on the elements, but here I’d prefer it if we used elements.swap(at: i, j) to make it explicit that that arguments are on indices and not elements.

Saagar Jha

P.S. The link to the manifesto has a typo (“manfiesto”).

···

On Apr 25, 2017, at 11:32, Ted Kremenek via swift-evolution <swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0173 "Add MutableCollection.swap(_:with:)" begins now and runs through April 28, 2017.

The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md
Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at:

https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md
Reply text

Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,
Ted (Review Manager)

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Xiaodi Wu) #3

Excellently written proposal, sensible motivation.

Only nit is that `with` is not required and is better off removed, IMO.
Here's why:

* "swap a and b" and "swap a with b" are equivalent statements
* there's no reason to use one phrasing for the free function and another
with this method; the difference is in taking indices vs. inout values, but
the relationship between a and b is preserved
* the former spelling emphasizes the symmetry of the operation, which I
believe is in line with API naming guidelines on labels
* the former spelling is also terser

···

On Tue, Apr 25, 2017 at 13:32 Ted Kremenek via swift-evolution < swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0173 "Add MutableCollection.swap(_:with:)" begins now and
runs through *April 28, 2017*.

The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at:

https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the
review manager. When replying, please try to keep the proposal link at the
top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md
Reply text

Other replies

What goes into a review?

The goal of the review process is to improve the proposal under review
through constructive criticism and, eventually, determine the direction of
Swift. When writing your review, here are some questions you might want to
answer in your review:

   - What is your evaluation of the proposal?
   - Is the problem being addressed significant enough to warrant a
   change to Swift?
   - Does this proposal fit well with the feel and direction of Swift?
   - If you have used other languages or libraries with a similar
   feature, how do you feel that this proposal compares to those?
   - How much effort did you put into your review? A glance, a quick
   reading, or an in-depth study?

More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,
Ted (Review Manager)
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Brent Royal-Gordon) #4

What is your evaluation of the proposal?

I agree that this method should be added, but I would suggest naming it differently.

Collection parameters have to be very carefully labeled because there are so many different associated types and they are often generic. For instance, an `Array<Int>` will have the same type for `Element`, `Index`, and `IndexDistance`. In the case of `swap`, one might misunderstand the call and believe the parameters are elements or counts, rather than indices.

The simplest solution would be to change it to:

  elements.swap(at: lo, with: hi)

However, you can also make an argument for:

  elements.swapAt(lo, hi)

This style is relatively rare, but is endorsed by the API Guidelines when the preposition applies to all of the arguments collectively.

Actually, we might want to consider extending the semantics of this method to:

  mutating func swapAt(_ indices: Index...)

The method would move `indices[0]` to `indices[1]`, `indices[1]`, to `indices[2]`, etc., finally moving `indices.last!` to `indices[0]`. This would do the same thing as the proposed method when there were two indices, but would provide other useful behaviors when there were more than two.

Is the problem being addressed significant enough to warrant a change to Swift?

Yes. Tightening the rules around simultaneous access is necessary, and this is a good way to do that.

Does this proposal fit well with the feel and direction of Swift?

Yes.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

I actually prefer Foundation's use of `exchange` to `swap`, which feels less jargon-y to me, but `swap` has precedent in the standard library.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Just a quick reading.

···

On Apr 25, 2017, at 11:32 AM, Ted Kremenek via swift-evolution <swift-evolution@swift.org> wrote:

--
Brent Royal-Gordon
Architechies


(Ben Cohen) #5

Hi everyone,

Regarding the naming of the method: we discussed this amongst the core team and the feeling is that elements.swapAt(i, j) is the appropriate choice. We have a specific exception in the API naming guidelines around a preposition at the end of a base name, when the preposition applies to both arguments, and this falls under that case.

(this also happens to avoid the need for a work around for the source compatibility issue with shadowing the existing swap method – which isn't the determining factor, but is a nice side-effect)

Cheers,
Ben

···

On Apr 25, 2017, at 11:32 AM, Ted Kremenek via swift-evolution <swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0173 "Add MutableCollection.swap(_:with:)" begins now and runs through April 28, 2017.

The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md
Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at:

https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md
Reply text

Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,
Ted (Review Manager)

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Howard Lovatt) #6

review of SE-0173 "Add MutableCollection.swap(_:with:)"

What is your evaluation of the proposal?
Good idea. Reads better than current global swap.
Is the problem being addressed significant enough to warrant a change to Swift?
Probably; It will become significant once control over ARC added, though not needed just yet.
Does this proposal fit well with the feel and direction of Swift?
Yes. The global functions do not sit well with Swift. Methods read much better. Could just eliminate current swap and not replace it though. You can write `(a[i], a[j]) = (a[j], a[i])` instead.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
Yes. People write similar functions in Java, though not in standard library.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Quick read

-- Howard.

···

On 26 Apr 2017, at 4:32 am, Ted Kremenek <kremenek@apple.com> wrote:

review of SE-0173 "Add MutableCollection.swap(_:with:)" begins now and runs through April 28, 2017.

The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at:

https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md

Reply text

Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
M


(Xiaodi Wu) #7

review of SE-0173 "Add MutableCollection.swap(_:with:)"

   - What is your evaluation of the proposal?

Good idea. Reads better than current global swap.

   - Is the problem being addressed significant enough to warrant a
   change to Swift?

Probably; It will become significant once control over ARC added, though
not needed just yet.

   - Does this proposal fit well with the feel and direction of Swift?

Yes. The global functions do not sit well with Swift. Methods read much
better. Could just eliminate current swap and not replace it though. You
can write `(a[i], a[j]) = (a[j], a[i])` instead.

Source compatibility is a serious goal now. There's no harm in the current
`swap`, and I use it. This is no rationale for breaking existing code.

···

On Wed, Apr 26, 2017 at 4:06 PM, Howard Lovatt via swift-evolution < swift-evolution@swift.org> wrote:

   - If you have used other languages or libraries with a similar
   feature, how do you feel that this proposal compares to those?

Yes. People write similar functions in Java, though not in standard
library.

   - How much effort did you put into your review? A glance, a quick
   reading, or an in-depth study?

Quick read

-- Howard.

On 26 Apr 2017, at 4:32 am, Ted Kremenek <kremenek@apple.com> wrote:

review of SE-0173 "Add MutableCollection.swap(_:with:)" begins now and
runs through *April 28, 2017*.

The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/
proposals/0173-swap-indices.md

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at:

https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the
review manager. When replying, please try to keep the proposal link at the
top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/
proposals/0173-swap-indices.md
Reply text

Other replies

What goes into a review?

The goal of the review process is to improve the proposal under review
through constructive criticism and, eventually, determine the direction of
Swift. When writing your review, here are some questions you might want to
answer in your review:

   - What is your evaluation of the proposal?
   - Is the problem being addressed significant enough to warrant a
   change to Swift?
   - Does this proposal fit well with the feel and direction of Swift?
   - If you have used other languages or libraries with a similar
   feature, how do you feel that this proposal compares to those?
   - How much effort did you put into your review? A glance, a quick
   reading, or an in-depth study?

M

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(John McCall) #8

review of SE-0173 "Add MutableCollection.swap(_:with:)"

What is your evaluation of the proposal?
Good idea. Reads better than current global swap.
Is the problem being addressed significant enough to warrant a change to Swift?
Probably; It will become significant once control over ARC added, though not needed just yet.
Does this proposal fit well with the feel and direction of Swift?
Yes. The global functions do not sit well with Swift. Methods read much better. Could just eliminate current swap and not replace it though. You can write `(a[i], a[j]) = (a[j], a[i])` instead.

Source compatibility is a serious goal now. There's no harm in the current `swap`, and I use it. This is no rationale for breaking existing code.

There are also performance — and, under ownership, eventually semantic — benefits to using swap over the tuple-assignment idiom.

John.

···

On Apr 26, 2017, at 6:36 PM, Xiaodi Wu via swift-evolution <swift-evolution@swift.org> wrote:
On Wed, Apr 26, 2017 at 4:06 PM, Howard Lovatt via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
Yes. People write similar functions in Java, though not in standard library.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Quick read

-- Howard.

On 26 Apr 2017, at 4:32 am, Ted Kremenek <kremenek@apple.com <mailto:kremenek@apple.com>> wrote:

review of SE-0173 "Add MutableCollection.swap(_:with:)" begins now and runs through April 28, 2017.

The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md
Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at:

https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0173-swap-indices.md
Reply text

Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
M

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution


(Howard Lovatt) #9

Regarding: "Source compatibility is a serious goal now. There's no harm in the current `swap`, and I use it. This is no rationale for breaking existing code."

Swap for arrays will have to go when ARC controls come in. You can't pass two parts of the same array to one function. The relevant bit from the Ownership manifesto is:

"Accessing a component of a value type through a subscript is treated as accessing the entire value, and so is considered to overlap any other access to the value."

-- Howard.

···

On 27 Apr 2017, at 8:36 am, Xiaodi Wu <xiaodi.wu@gmail.com> wrote:

Source compatibility is a serious goal now. There's no harm in the current `swap`, and I use it. This is no rationale for breaking existing code.


(Xiaodi Wu) #10

Yes, for arrays. I use it to swap two variables `a` and `b`.

var a = 1
var b = 2
swap(&a, &b)
···

On Wed, Apr 26, 2017 at 6:02 PM, Howard Lovatt <howard.lovatt@gmail.com> wrote:

Regarding: "Source compatibility is a serious goal now. There's no harm in
the current `swap`, and I use it. This is no rationale for breaking
existing code."

Swap for arrays will have to go when ARC controls come in. You can't pass
two parts of the same array to one function. The relevant bit from the
Ownership manifesto is:

"Accessing a component of a value type through a subscript is treated as
accessing the entire value, and so is considered to overlap any other
access to the value."


(TJ Usiyan) #11

+1 with a small concern

'This version will forward to the free function.'

Is that a mistake? Wouldn't the free function forward to the added method?

TJ

···

On Apr 26, 2017, at 16:33, Xiaodi Wu via swift-evolution <swift-evolution@swift.org> wrote:

On Wed, Apr 26, 2017 at 6:02 PM, Howard Lovatt <howard.lovatt@gmail.com> wrote:
Regarding: "Source compatibility is a serious goal now. There's no harm in the current `swap`, and I use it. This is no rationale for breaking existing code."

Swap for arrays will have to go when ARC controls come in. You can't pass two parts of the same array to one function. The relevant bit from the Ownership manifesto is:

"Accessing a component of a value type through a subscript is treated as accessing the entire value, and so is considered to overlap any other access to the value."

Yes, for arrays. I use it to swap two variables `a` and `b`.

var a = 1
var b = 2
swap(&a, &b)

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(John McCall) #12

+1 with a small concern

'This version will forward to the free function.'

Is that a mistake? Wouldn't the free function forward to the added method?

I'm not sure why this is there. The implementation of the method is not part of the library specification. If it forwards to the free function for some reason, it'll be via internal implementation details that certainly don't need to be described in this proposal.

Anyway, the free function can't forward to the method, it has to handle cases that have nothing to do with collections.

John.

···

On Apr 26, 2017, at 11:44 PM, gs. via swift-evolution <swift-evolution@swift.org> wrote:

TJ

On Apr 26, 2017, at 16:33, Xiaodi Wu via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

On Wed, Apr 26, 2017 at 6:02 PM, Howard Lovatt <howard.lovatt@gmail.com <mailto:howard.lovatt@gmail.com>> wrote:
Regarding: "Source compatibility is a serious goal now. There's no harm in the current `swap`, and I use it. This is no rationale for breaking existing code."

Swap for arrays will have to go when ARC controls come in. You can't pass two parts of the same array to one function. The relevant bit from the Ownership manifesto is:

"Accessing a component of a value type through a subscript is treated as accessing the entire value, and so is considered to overlap any other access to the value."

Yes, for arrays. I use it to swap two variables `a` and `b`.

var a = 1
var b = 2
swap(&a, &b)

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Howard Lovatt) #13

I use:

    (b, a) = (a, b)

So I wouldn't miss it.

···

On Thu, 27 Apr 2017 at 9:33 am, Xiaodi Wu <xiaodi.wu@gmail.com> wrote:

On Wed, Apr 26, 2017 at 6:02 PM, Howard Lovatt <howard.lovatt@gmail.com> > wrote:

Regarding: "Source compatibility is a serious goal now. There's no harm
in the current `swap`, and I use it. This is no rationale for breaking
existing code."

Swap for arrays will have to go when ARC controls come in. You can't pass
two parts of the same array to one function. The relevant bit from the
Ownership manifesto is:

"Accessing a component of a value type through a subscript is treated as
accessing the entire value, and so is considered to overlap any other
access to the value."

Yes, for arrays. I use it to swap two variables `a` and `b`.

var a = 1
var b = 2
swap(&a, &b)

--

-- Howard.


(Ben Cohen) #14

It was there purely to explain why it was necessary to add an otherwise seemingly pointless method to MutableCollection. Needed because of https://bugs.swift.org/browse/SR-4660 – in short, if you have a method on self, you cannot call the free function of the same name without full qualification, even if argument labels or different types make it clear which one was intended. So adding swap(_:with:) to MutableCollection would break code currently calling swap(&a,&b), which would need to change to Swift.swap(&a,&b). So we would have to put in an extra swap method on MutableCollection, that in turn calls Swift.swap, for source compatibility reasons.

However, if we adopt the preferred base name swapAt instead, this won’t be necessary.

(it will be necessary to add similar workarounds for min and max to String when it becomes a Collection, though...)

···

On Apr 26, 2017, at 9:12 PM, John McCall via swift-evolution <swift-evolution@swift.org> wrote:

Is that a mistake? Wouldn't the free function forward to the added method?

I'm not sure why this is there. The implementation of the method is not part of the library specification. If it forwards to the free function for some reason, it'll be via internal implementation details that certainly don't need to be described in this proposal.


(John McCall) #15

Is that a mistake? Wouldn't the free function forward to the added method?

I'm not sure why this is there. The implementation of the method is not part of the library specification. If it forwards to the free function for some reason, it'll be via internal implementation details that certainly don't need to be described in this proposal.

It was there purely to explain why it was necessary to add an otherwise seemingly pointless method to MutableCollection. Needed because of https://bugs.swift.org/browse/SR-4660 – in short, if you have a method on self, you cannot call the free function of the same name without full qualification, even if argument labels or different types make it clear which one was intended. So adding swap(_:with:) to MutableCollection would break code currently calling swap(&a,&b), which would need to change to Swift.swap(&a,&b). So we would have to put in an extra swap method on MutableCollection, that in turn calls Swift.swap, for source compatibility reasons.

Oh, right, that. I had forgotten about that, sorry. :slight_smile:

John.

···

On Apr 27, 2017, at 11:27 AM, Ben Cohen <ben_cohen@apple.com> wrote:

On Apr 26, 2017, at 9:12 PM, John McCall via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

However, if we adopt the preferred base name swapAt instead, this won’t be necessary.

(it will be necessary to add similar workarounds for min and max to String when it becomes a Collection, though...)