[swift-evolution] [Review] SE-0065 A New Model for Collections and Indices
Dave Abrahams
dabrahams at apple.com
Wed Apr 13 13:24:55 CDT 2016
on Wed Apr 13 2016, Tony Parker <anthony.parker-AT-apple.com> wrote:
> 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> 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].
>
> 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
This is true, but we need a way to deal with these cases as well.
> I think using the form prefix here will confuse this case with the
> others, when they are meaningfully different.
I don't think it is confusing, since in these cases the thing being
mutated is always explicitly passed inout (prefixed with &). Obvious,
reasonable people can disagree on this point. IMO, though, if one wants
to avoid “form” here IMO we should have an equally-strong and clear
guideline to replace it for this sort of situation, as it's not
particularly far-fetched to imagine this will come up again.
> 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.
Or if you see it enough that it becomes natural and you understand what
it means. The same applies to “formUnion,” though. (“InPlace” didn't
have this problem, but... oh, well).
IMO, either “formXXX” is good enough for these kinds of situations, or
it isn't. If it isn't, we should take it out of the guidelines and not
use it for Set. If it is good enough, we should settle on it and let it
proliferate.
> Plus, as I said, the form prefix implies a mutation of the wrong
> argument.
Not really. “Form” isn't really specific about what's being
mutated. You can form a bowl out of clay or you can form a blockade.
Frankly, I agree with the many people on this list who have said it
carries an implication of non-reflexivity... I just find it to be an
acceptably weak implication; weak enough that we can get away with using
it reflexively.
>
> 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.
Oh, I get it.
> Or we could use retreat. =) There are other pairs of words that work
> as well, like “increment/decrement”.
Yeah, unfortunately those carry an incorrect implication when the
indices are numbers, because, e.g. the collection might be offsetting
the number by 2 for each position. One could of course argue that using
numbers that way as indices was a bad design choice.
I'll have to think about that idea again. We considered and rejected it
for a reason, but it might not be a really strong one. Thanks for
bringing it up.
> It’s rather an implementation detail of the index and collection what
> exactly these do,
I think it's fundamental, but I'm not sure that point makes any
difference to our discussion.
> 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.
Obviously! The proposed names aren't simply “form.” “Form-” serves
the same purpose as “-ed” or “-ing” in verb-based names. Not a
placeholder, but a modifier used to produce the right semantic
implication.
> Therefore, to me it feels like a placeholder or fallback.
That conclusion still doesn't add up for me. However, I do want to give
increment/decrement due consideration.
Thanks again,
> - 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
> https://lists.swift.org/mailman/listinfo/swift-evolution
>
--
Dave
More information about the swift-evolution
mailing list