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

[Dave Abrahams]

Thanks for your review, Tony!

on Mon Apr 11 2016, Tony Parker <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
(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].

> 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.

> 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.

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