[swift-evolution] [Review] SE-0065 A New Model for Collections and Indices

Tony Parker anthony.parker at apple.com
Wed Apr 13 10:45:25 CDT 2016 

> On Apr 12, 2016, at 3:43 PM, Dave Abrahams via swift-evolution <swift-evolution at swift.org> wrote:
> 
> 
> Thanks for your review, Tony!
> 
> on Mon Apr 11 2016, Tony Parker <swift-evolution at swift.org <mailto:swift-evolution at swift.org>> wrote:
> 
>>> On Apr 10, 2016, at 2:41 PM, Chris Lattner via swift-evolution <swift-evolution at swift.org> wrote:
>>> 
>>> Hello Swift community,
>>> 
>>> The review of "A New Model for Collections and Indices" begins now and runs through April 18th. The proposal is available here:
>> 
>>> 
>>> 	https://github.com/apple/swift-evolution/blob/master/proposals/0065-collections-move-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.
>>> 
>>> 
>>> 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?
>> 
>> I agree with the general direction and scope of the proposal, but I
>> think the names could use some changes. Specifically, I don’t think
>> the fallback to ‘form’ is required. 
> 
> It's not a fallback whatsoever.  The updated guidelines referenced from
> https://github.com/apple/swift-evolution/blob/master/proposals/0059-updated-set-apis.md <https://github.com/apple/swift-evolution/blob/master/proposals/0059-updated-set-apis.md>
> (which was accepted this morning; announcement forthcoming) make the
> “form” prefix a first-class citizen that one chooses based on the
> noun-ness of the underlying operation.  Furthermore, *treating*
> “formXXX” as a fallback and avoiding it will continue to strengthen the
> sense that it's not something we should normally use, leading to more
> naming arguments in the future.  It's a strong guideline and as long as
> we have it, we shouldn't be afraid to apply it, thereby increasing
> uniformity and predictability.
> 
> [To all my fellow “InPlace” lovers out there: yes, another guideline
> might be more optimal, but this is the guideline we have/can get].
> 

In other cases, the mutating pair of methods refer to the receiver, not the argument. 

x = y.union(z) // new value x
y.formUnion(z) // mutates y, not z

x = y.successor(z) // new value x
y.formSuccessor(z) // mutates z (or replaces), not y

I think using the form prefix here will confuse this case with the others, when they are meaningfully different.

>> It would be a significant readability improvement to use a meaningful
>> verb to describe the action of altering the argument. The methods that
>> create new indices probably need a label on the first argument,
>> because otherwise it looks as if the IndexDistance is what is
>> described by ‘index’.
>> 
>> Proposed:
>> 
>>  func successor(of i: Index) -> Index
>>  func formSuccessor(i: inout Index)
>> 
>> Instead, I suggest:
>> 
>>  func successor(of i : Index) -> Index
>>  func advance(i: inout Index)
> 
> Why is that an improvement?  It loses the correspondence between the
> operations, which are still a mutating/nonmutating pair.  What's it got
> to recommend it?  I have the same question about all of the suggestions
> below.

It’s an improvement because it is much easier to read and understand what it means. The phrase “form successor” only makes sense if you dive into the naming guidelines to see why we have the “form” prefix in the first place. Plus, as I said, the form prefix implies a mutation of the wrong argument.

> 
>> Proposed:
>> 
>>  func index(n: IndexDistance, stepsFrom i: Index) -> Index
>>  func index(n: IndexDistance, stepsFrom i: Index, limitedBy limit: Index) -> Index
>>  func formIndex(n: IndexDistance, stepsFrom i: inout Index)
>>  func formIndex(n: IndexDistance, stepsFrom i: inout Index, limitedBy limit: Index)
>> 
>> Suggested (taking into account Nate’s suggestion of reversing the order):
>> 
>>  func index(startingAt i: Index, movedBy n: IndexDistance) -> Index
>>  func index(startingAt i: Index, movedBy n: IndexDistance, limitedBy limit: Index) -> Index
> 
> I find Nate Cook's concerns about the use of “index” here (a mental
> clash with unrelated methods having the same basename) especially
> convincing.  So I think I want to look for other names for these.
> 
>> 
>>  func move(i : inout Index, by n: IndexDistance) 
>>  func move(i : inout Index, by n: IndexDistance, limitedBy limit: Index)
>> 
>> Proposed:
>> 
>>  func predecessor(of i: Index) -> Index
>>  func formPredecessor(i: inout Index)
>> 
>> Suggested:
>> 
>>  func predecessor(of i: Index) -> Index
>>  func reverse(i: inout Index)
>> 
>> I think reversing an index has some nice symmetry with reversing a
>> sequence, but if it seems to confusing, then replace advance and
>> reverse with ‘moveForward’ and ‘moveBackward’.
> 
> Yeah, I don't think moving an index one step backwards could reasonably
> be called “reversing” it.  “moveBackward” is reasonable, if one wanted
> have to break the relationship with predecessor.
> 

Reverse is the best opposite we have of advance, so it makes sense to me. Or we could use retreat. =) There are other pairs of words that work as well, like “increment/decrement”. It’s rather an implementation detail of the index and collection what exactly these do, but conceptually modeling them as increment and decrement would likely make intuitive sense to most CS 101 students.

The reason that advance/reverse and increment/decrement work better is because they are active words that describe what happens to the argument, which has no other label. “form” describes little about the action that is actually taken on the argument. Therefore, to me it feels like a placeholder or fallback.

- Tony

>> 
>> 
>> - Tony
>> 
>>> 	* 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 you 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 at swift.org
>>> https://lists.swift.org/mailman/listinfo/swift-evolution
>> 
>> _______________________________________________
>> swift-evolution mailing list
>> swift-evolution at swift.org
>> https://lists.swift.org/mailman/listinfo/swift-evolution
> 
> -- 
> Dave
> 
> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org <mailto:swift-evolution at swift.org>
> https://lists.swift.org/mailman/listinfo/swift-evolution <https://lists.swift.org/mailman/listinfo/swift-evolution>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20160413/e1fd7ac0/attachment.html>

More information about the swift-evolution mailing list