[Review] SE-0120: Revise 'partition' Method Signature


(Chris Lattner) #1

Hello Swift community,

The review of "SE-0120: Revise ‘partition' Method Signature" begins now and runs through July 19. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0120-revise-partition-method.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.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and contribute to 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,

-Chris Lattner
Review Manager


New 'partioned(by:)' method
(Jacob Bandes-Storch) #2

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0120-revise-partition-method.md

        * What is your evaluation of the proposal?

+1, although I don't think the functions should be marked with
@discardableResult. The partition method is hardly useful if you don't know
where the partition index is.

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

Somewhat. This API isn't commonly used (I've never used it), but APIs in
the standard library deserve to make sense and be useful :slight_smile:

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

Yes, the new API seems to feel "Swifty", and conform to the API design
guidelines.

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

N/A

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

Brief reading of the proposal and a look at the existing API. I've never
used this API in Swift.


(Paul Cantrell) #3

The proposal is clearly an improvement over the status quo.

A naming concern, which I apologize for not getting in before the review period:

In Ruby (and I think some other languages as well), “partition” returns two collections, one with the included elements and one with the excluded. That’s a useful flavor of the method to have. I’ve added it in an extension myself in a project or two.

Does this proposal leave room for the two-collection variant if we want to add it later?

If it were to honor the existing term of art, the natural name for it would be “partitioned(by:)”:

    mutating func partitioned(by: …) -> ([Self.Iterator.Element], [Self.Iterator.Element])

However, naming the in-place reordering method “partition” as this proposal does would suggest instead that “partitioned(by:)” is instead its non-mutating counterpart:

    mutating func partitioned(by: …) -> ([Self.Iterator.Element], Index)

Overloading on return type is dicey business, especially when the type resolver has to peer inside a tuple. Could these two flavors coexist peacefully? Will this be confusing? Are we painting ourselves into a corner?

Cheers,

Paul

···

On Jul 12, 2016, at 1:12 PM, Chris Lattner <clattner@apple.com> wrote:

Hello Swift community,

The review of "SE-0120: Revise ‘partition' Method Signature" begins now and runs through July 19. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0120-revise-partition-method.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.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and contribute to 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,

-Chris Lattner
Review Manager

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


(Karoy Lorentey) #4

  * What is your evaluation of the proposal?

+1; I like the new design, and I think once in a blue moon I may actually use the new API.

I remember looking up partition in the stdlib docs, sighing and moving on once or twice. It seemed easier to write an explicit loop than to hammer my problem into the shape of the old API.

Nitpick: In the "Impact on Existing Code", the sample code for the replacement ignores the return value.

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

I guess so; although it is a fix for a super minor problem, the proposed change is equally nonintrusive.

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

Sure.

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

As Paul noted, Haskell and Ruby have nonmutating methods for the same thing. It's fine to delay adding other variants for now.

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

Quick reading plus superficial research.

···

--
Karoly
@lorentey

On 2016-07-12 18:12:18 +0000, Chris Lattner via swift-evolution said:

Hello Swift community,

The review of "SE-0120: Revise ‘partition' Method Signature" begins now and runs through July 19. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0120-revise-partition-method.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.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and contribute to 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,

-Chris Lattner
Review Manager

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

--
Károly
@lorentey


(David Rönnqvist) #5

   * What is your evaluation of the proposal?

+1. After seeing an example of the current partition method (which I hadn't heard of before that) on the mailing list I tried to use it in our code, but it was too specialized for it to be a good fit. The new proposed method is more flexible and becomes a good building block for other code.

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

Yes. It's fairly small, but not insignificant.

   * 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?

N/A

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

Read the proposal (and tried to make use of the existing partition method once after having heard about it).


(Dave Abrahams) #6

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0120-revise-partition-method.md

        * What is your evaluation of the proposal?

+1, although I don't think the functions should be marked with
@discardableResult. The partition method is hardly useful if you don't know
where the partition index is.

That's a very good point.

···

on Tue Jul 12 2016, Jacob Bandes-Storch <swift-evolution@swift.org> wrote:

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

Somewhat. This API isn't commonly used (I've never used it), but APIs in
the standard library deserve to make sense and be useful :slight_smile:

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

Yes, the new API seems to feel "Swifty", and conform to the API design
guidelines.

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

N/A

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

Brief reading of the proposal and a look at the existing API. I've never
used this API in Swift.
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

--
Dave


(Dave Abrahams) #7

The proposal is clearly an improvement over the status quo.

A naming concern, which I apologize for not getting in before the review period:

In Ruby (and I think some other languages as well), “partition”
returns two collections, one with the included elements and one with
the excluded. That’s a useful flavor of the method to have. I’ve added
it in an extension myself in a project or two.

Does this proposal leave room for the two-collection variant if we
want to add it later?

Yes.

  let p = x.partition { ... }
  excluded = x[x.startIndex..<p]
  included = x[p..<x.endIndex]

If it were to honor the existing term of art, the natural name for it would be “partitioned(by:)”:

    mutating func partitioned(by: …) -> ([Self.Iterator.Element], [Self.Iterator.Element])

Yes, we are interested in more algorithms,
but they are out-of-scope for Swift 3.

However, naming the in-place reordering method “partition” as this
proposal does would suggest instead that “partitioned(by:)” is instead
its non-mutating counterpart:

    mutating func partitioned(by: …) -> ([Self.Iterator.Element], Index)

Yes a non-mutating one. The above might return.
a pair of ArraySlice, with some way to retrieve the underlying Array.

Overloading on return type is dicey business, especially when the type
resolver has to peer inside a tuple. Could these two flavors coexist
peacefully? Will this be confusing? Are we painting ourselves into a
corner?

I don't see any such overloading here.

···

on Tue Jul 12 2016, Paul Cantrell <swift-evolution@swift.org> wrote:

Cheers,

Paul

On Jul 12, 2016, at 1:12 PM, Chris Lattner <clattner@apple.com> wrote:

Hello Swift community,

The review of "SE-0120: Revise ‘partition' Method Signature" begins now and runs through July 19. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0120-revise-partition-method.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.

What goes into a review?

The goal of the review process is to improve the proposal under
review through constructive criticism and contribute to 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,

-Chris Lattner
Review Manager

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

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

--
Dave


(Paul Cantrell) #8

Does this proposal leave room for the two-collection variant if we
want to add it later?

Overloading on return type is dicey business, especially when the type
resolver has to peer inside a tuple. Could these two flavors coexist
peacefully? Will this be confusing? Are we painting ourselves into a
corner?

I don't see any such overloading here.

These two methods differ only by return type:

  mutating func partitioned(by: …) -> ([Self.Iterator.Element], [Self.Iterator.Element])
  mutating func partitioned(by: …) -> ([Self.Iterator.Element], Index)

These are the two methods a hypothetical post-3.0 stdlib would end up with given two fairly reasonable assumptions:

1. that we’d like a non-mutating variant of the “partition” method proposed here, and

2. that we’d like the stdlib to provide the two-collection “partition” as well.

To be clear, I’m _not_ proposing adding either of those things now. I’m only wondering whether using the name “partition” for the method under discussion now paints us into a corner later.

But…

  mutating func partitioned(by: …) -> ([Self.Iterator.Element], Index)

Yes a non-mutating one. The above might return a pair of ArraySlice, with some way to retrieve the underlying Array.

Right, we’d be in the clear if that were the _only_ non-mutating counterpart of “partition,” i.e. if we only had this method:

   mutating func partitioned(by: …) -> (ArraySlice<Self.Iterator.Element>, ArraySlice<Self.Iterator.Element>)

…instead of the two I listed above.

And this slice variant does seem to provide all the advantages of both the two above, so I think it could be the only “partitioned” method. My concern is thus addressed. Thanks.

Cheers,

Paul

···

On Jul 12, 2016, at 7:27 PM, Dave Abrahams via swift-evolution <swift-evolution@swift.org> wrote:
on Tue Jul 12 2016, Paul Cantrell <swift-evolution@swift.org> wrote:


(Nate Cook) #9

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0120-revise-partition-method.md

       * What is your evaluation of the proposal?

+1, although I don't think the functions should be marked with
@discardableResult. The partition method is hardly useful if you don't know
where the partition index is.

That's a very good point.

Totally—that's a deviation from the existing API that I didn't explain. I'll revise the proposal to remove the attribute.

···

On Jul 12, 2016, at 7:06 PM, Dave Abrahams via swift-evolution <swift-evolution@swift.org> wrote:
on Tue Jul 12 2016, Jacob Bandes-Storch <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

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

Somewhat. This API isn't commonly used (I've never used it), but APIs in
the standard library deserve to make sense and be useful :slight_smile:

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

Yes, the new API seems to feel "Swifty", and conform to the API design
guidelines.

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

N/A

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

Brief reading of the proposal and a look at the existing API. I've never
used this API in Swift.
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

--
Dave

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