[Review] SE-0006 Apply API Guidelines to the Standard Library


(Douglas Gregor) #1

Hello Swift community,

The review of SE-0006 "Apply API Guidelines to the Standard Library" begins now and runs through January 31, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.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/0006-apply-api-guidelines-to-the-standard-library.md
Reply text

Other replies
<https://github.com/apple/swift-evolution#what-goes-into-a-review-1>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,

-Doug Gregor

Review Manager


(plx) #2

Really quickly, I think this is a mistake:

SequenceType.minElement() => .min(), .maxElement() => .max().
extension Sequence {
- public func minElement(
+ public func minElement(
     @noescape isOrderedBefore: (Iterator.Element, Iterator.Element) throws -> Bool
   ) rethrows -> Iterator.Element?

- public func maxElement(
+ public func maxElement(
     @noescape isOrderedBefore: (Iterator.Element, Iterator.Element) throws -> Bool
   ) rethrows -> Iterator.Element?
}

extension Sequence where Iterator.Element : Comparable {
- public func minElement() -> Iterator.Element?
+ public func minElement() -> Iterator.Element?

- public func maxElement() -> Iterator.Element?
+ public func maxElement() -> Iterator.Element?
}
…shouldn’t those diffs have them changed to be `min()` and `max()`?

···

On Jan 22, 2016, at 3:02 PM, Douglas Gregor via swift-evolution <swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0006 "Apply API Guidelines to the Standard Library" begins now and runs through January 31, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.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/0006-apply-api-guidelines-to-the-standard-library.md
Reply text

Other replies
<https://github.com/apple/swift-evolution#what-goes-into-a-review-1>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,

-Doug Gregor

Review Manager

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


(Trent Nadeau) #3

- MutableSliceable was removed. Use CollectionType where SubSequence :
   MutableCollectionType instead.

With the new protocol names, this bullet should be the following in both
places:

   - MutableSliceable was removed. Use Collection where SubSequence :
   MutableCollection instead.

···

On Fri, Jan 22, 2016 at 4:02 PM, Douglas Gregor via swift-evolution < swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0006 "Apply API Guidelines to the Standard Library"
begins now and runs through January 31, 2016. The proposal is available
here:

https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.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/0006-apply-api-guidelines-to-the-standard-library.md

Reply text

Other replies

<https://github.com/apple/swift-evolution#what-goes-into-a-review-1>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,

-Doug Gregor

Review Manager

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

--
Trent Nadeau


(Ben Rimmington) #4

Proposal link:

<https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.md>

The `Sequence.split` methods both have a `maxSplits: Int = Int.max` parameter, but this parameter is unlabelled (i.e. a local-only name) in one of the methods. This is more obvious when looking at the documentation [1], which doesn't follow the Swift convention for local/external parameter names.

[1]: <https://developer.apple.com/library/ios/documentation/Swift/Reference/Swift_SequenceType_Protocol/>

Also, the naming convention proposed by David Owens II in the "named parameters" thread [2] would allow for default parameter values to be added later. For example, see the `SequenceType.joinWithSeparator` methods.

[2]: <https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160118/007234.html>

-- Ben


(Jacob Bandes-Storch) #5

Proposal link:
https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.md

What is your evaluation of the proposal?

I believe the Unmanaged -> UnsafeReference / OpaquePointer changes are too
large to be part of this proposal; they should be reviewed separately.
There was some discussion about this in a thread last month. Particularly,
I'm concerned that there is still a gap between UnsafePointer<Void> and
OpaquePointer APIs. I submitted a related proposal, SE-0017, which was
never scheduled for review:

https://github.com/apple/swift-evolution/blob/master/proposals/0017-convert-unmanaged-to-use-unsafepointer.md

I'm also a little concerned about Integer, SignedInteger, and
UnsignedInteger, which are protocols but might look like concrete integer
types to the reader. My first thought is that "Integral" would be better
for this, but it doesn't match the "noun or capability" note in the API
design guidelines.

Otherwise, I'm generally fine with the proposed changes.

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

Swift?

Yes; consistency is important.

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

Yes, by definition.

How much effort did you put into your review? A glance, a quick reading,

or an in-depth study?

Fairly in-depth.

Jacob

···

On Fri, Jan 22, 2016 at 1:02 PM, Douglas Gregor via swift-evolution < swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0006 "Apply API Guidelines to the Standard Library"
begins now and runs through January 31, 2016. The proposal is available
here:

https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.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/0006-apply-api-guidelines-to-the-standard-library.md

Reply text

Other replies

<https://github.com/apple/swift-evolution#what-goes-into-a-review-1>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,

-Doug Gregor

Review Manager

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


(Radek Pietruszewski) #6

Hello all,

Just like with SE-0005 <https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160125/007658.html>, I’m overwhelmingly for this proposal. The Guidelines, as a whole do a great job at making Swift APIs more consistent and clearer, and applying them to the Swift stdlib is an important first step.

* * *

Nitpicks, criticisms and suggestions:

== precondition was renamed to require ==

This might be confusing to newcomers, as many languages use the word `require` as a keyword for what we'd call `import`. Although a bit more technical, `precondition` is unambiguous and still easily understandable. I feel like `required` does more damage than good.

== Removed Type from protocol names ==

Perhaps I’ve missed some discussion about this and I don’t see the context, but I’m not sure this is a positive change.

I fear this might be confusing in practice, at least in some contexts. For example, there's nothing signifying that "Boolean" or "Integer" are protocols and not actual types. Same with “Sequence”, “OptionSet”, etc. Perhaps it doesn't matter because everyone will naturally go for `Bool`, `Int`, and `Array` anyway. But I can imagine a lot of confusion if someone tried that anyway, or perhaps saw that in the autocompletion, or the standard library browser (with no intention of using the protocol).

I’m all for removing unnecessary noise and verbosity, but I think I would err on explicitness side here. It seemed like the -able/-Type convention did a good job disambiguating types you can actually instantiate from protocols, with very little “verbosity cost”.

== sort() => sorted(), sortInPlace() => sort() etc ==

I’m torn on this.

Frankly, I find both the “foo/fooInPlace” and “bar/barred” conventions awkward. Both seem weird. “InPlace” isn’t something I recall seeing anywhere else in API naming, and seems a bizarre way of signifying mutability. “-ed” doesn’t work with all words, so you sometimes have to go with “-ing”, or give up and cry. And then you have inconsistency that “-InPlace” doesn’t seem to have. Also, -ed/-ing can sometimes be difficult to write, especially for non-natives because of the “last letter is doubled” rule for some words.

But my biggest problem with this change is that IMHO we should encourage to use the transforming (non-mutating) variants by default. One way to achieve this as an API designer and slightly push people towards doing what’s considered best practice is to make the preferable variant easier to type. This might be a subtle change, but I think it matters. Before, if you really wanted to mutate something in place, you had to do that extra little bit of work typing “sortInPlace”, whereas what would be preferable most of the time had a simpler, shorter form: “sort” and would appear earlier in autocomplete.

== -ings in argument names ==

I’ve noticed these few tweaks in naming:

- mutating func removeAll(keepCapacity keepCapacity: Bool = false)
+ mutating func removeAll(keepingCapacity keepingCapacity: Bool = false)

public func transcode<...>(...
- stopOnError: Bool
+ stoppingOnError: Bool
) -> Bool

+ public init(allocatingCapacity count: Int)

I'm against this change. While I'm not fully convinced of the -ed/-ing rule for methods and properties, it does an important job by conveying the non-mutating semantics of a symbol described. In case of argument names, this rationale no longer applies.

The only reason to write "stoppingOnError" instead of "stopOnError" is to make method invocations sound more like real English sentences. This is the conventional Objective-C thinking the Guidelines largely step back from. In my opinion, this is futile and provides no readability benefits in this context. Method invocations are _not_ sentences. It's not English, it's code. And while making method names blatantly gramatically incorrect doesn't help readability, neither does forcing `-ing` endings to all boolean function arguments.

The only thing it does is it adds a few extra characters, an extra word ending the reader has to parse and understand. I know that it's a non-goal to make Swift code as terse as possible, and I'm not arguing for that. But the Guidelines seem to agree that adding extra verbosity _without a good reason_ is a bad thing. Because every extra word and symbol in code just adds to the cognitive load of the reader. And when it doesn't serve a purpose, it just decreases the signal-to-noise ratio.

Plus, as mentioned before, `-ed/-ing` can be tricky to spell for non-natives. This might not be a big deal, but given that this change provides no benefits, it's one more tiny thing you have to be careful not to get wrong when writing Swift.

And it's unnecessary:

   removeAll(keepCapacity: true)
   transcode(foo, bar, stopOnError: true)

Are just as clear and readable as:

   removeAll(keepingCapacity: true)
   transcode(foo, bar, stoppingOnError: true)

And the former isn't gramatically incorrect, because this isn't a sentence.

Apologies for nitpicking on this tiniest possible detail. I just care a lot that we don't create a precedent of trying to make everything sound like English unnecessarily and add verbosity bit by bit.

* * *

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?

Yes, and yes, with small details still worth reconsidering.

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

I’ve read the whole proposal, as well as the related proposals, and read the thread for this review.

Cross-linking to my SE-0005 review: https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160125/007658.html

Thanks,
— Radek

···

On 22 Jan 2016, at 22:02, Douglas Gregor via swift-evolution <swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0006 "Apply API Guidelines to the Standard Library" begins now and runs through January 31, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.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/0006-apply-api-guidelines-to-the-standard-library.md
Reply text

Other replies
<https://github.com/apple/swift-evolution#what-goes-into-a-review-1>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,

-Doug Gregor

Review Manager

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


(Janosch Hildebrand) #7

I'm generally in favor of the proposed changes. I'll just note some minor points and disagreements:

* Like I mentioned in my SE-0023 review, I would be OK with keeping the "Type" suffix for protocols but have no strong preference.

* I'm in favor of keeping `precondition()`. `require()` might be easier to grasp at first but personally I really came to like `precondition()`.
It fells both precise and I prefer the passive `precondition()` to the active `require()` for this case. To me it fits the primary meaning better; stating an API contract. The fact that the condition is actively checked is secondary to that.

* I also agree with Radosław in that I prefer `removeAll(keepCapacity: Bool)` to `removeAll(keepingCapacity: Bool)`.

* What is the rationale for moving `unsafeUnwrap` into Optional but not `unsafeAddressOf` into AnyObject? I can certainly see the safety argument against moving it but I don't see how that would apply to `unsafeAddressOf` but not `unsafeUnwrap`?

* `EnumeratedSequence` and `Repeated` feel weird to me. They make sense given the API guidelines and the previous `EnumerateSequence` and `Repeat` were a bit clunky as well but these somehow feel a bit worse... That might be wholly subjective though and I don't really have a good suggestion. The only thing that came to mind was `EnumerationSequence` and `Repetition` but I'm not overly fond of those either especially not to the point of deviating from the norm...

* This is not a disagreement but I'd be interested in hearing the reasons for replacing Generator(Type) with Iterator(Protocol) if someone finds the time. I can speculate of course but it's probably easier for someone to give me a short summary :slight_smile:

* Typo:

+ public func take() -> Memory // Should be Pointee

- Janosch


(Ben Rimmington) #8

Proposal link:

<https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.md>

precondition was renamed to require.

In the Standard Library, "- Requires:" appears in documentation comments more often than "- Precondition:" or "- Postcondition:", so renaming the function makes sense.

However, I think the Xcode Markup Formatting Reference should be updated to reflect this usage. The current description is:

Use the field to add requirements such as frameworks for using the symbol.

/**
An example of using the requires field

- requires: Contacts framework
- requires: OS X version 10.11 or better
*/

<https://developer.apple.com/library/mac/documentation/Xcode/Reference/xcode_markup_formatting_ref/Requires.html>

-- Ben


(Dmitri Gribenko) #9

Thanks, fixed!

Dmitri

···

On Fri, Jan 22, 2016 at 1:27 PM, plx via swift-evolution <swift-evolution@swift.org> wrote:

Really quickly, I think this is a mistake:

SequenceType.minElement() => .min(), .maxElement() => .max().

extension Sequence {
- public func minElement(
+ public func minElement(
     @noescape isOrderedBefore: (Iterator.Element, Iterator.Element) throws
-> Bool
   ) rethrows -> Iterator.Element?

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr@gmail.com>*/


(Dmitri Gribenko) #10

Thank you, fixed that!

Dmitri

···

On Fri, Jan 22, 2016 at 4:14 PM, Trent Nadeau via swift-evolution <swift-evolution@swift.org> wrote:

MutableSliceable was removed. Use CollectionType where SubSequence :
MutableCollectionType instead.

With the new protocol names, this bullet should be the following in both
places:

MutableSliceable was removed. Use Collection where SubSequence :
MutableCollection instead.

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr@gmail.com>*/


(Guillaume Lessard) #11

Hello,

I disagree with the following change to UnsafeMutablePointer:
- public static func alloc(num: Int) -> UnsafeMutablePointer<Pointee>
+ public init(allocatingCapacity count: Int)

This would make it the only constructor in any of OpaquePointer, UnsafePointer, UnsafeMutablePointer and UnsafeReference to have the side-effect of allocating memory. All the others are relatively cheap transformations on pointer values, and get used a lot for typecasting. An allocating constructor would be less locatable among such uses of typecasting-via-constructor. The memory-allocating static method has the merit of sticking out, and pairs nicely with the necessary deallocation call, like the malloc/free pair.

Cheers,
Guillaume Lessard


(Joe Groff) #12

This all probably deserves a separate discussion from the overall umbrella proposal. Another thing to consider here is whether the logic to allocate an array of values really belongs on UnsafeMutablePointer—it seems like a better fit for UnsafeMutableBufferPointer, whose whole job is to reference an array of objects in memory. Currently, you need to allocate the memory using UnsafeMutablePointer.alloc, then immediately wrap it in a BufferPointer with the same count, which is a bit awkward.

-Joe

···

On Jan 22, 2016, at 11:31 PM, Guillaume Lessard via swift-evolution <swift-evolution@swift.org> wrote:

Hello,

I disagree with the following change to UnsafeMutablePointer:
- public static func alloc(num: Int) -> UnsafeMutablePointer<Pointee>
+ public init(allocatingCapacity count: Int)

This would make it the only constructor in any of OpaquePointer, UnsafePointer, UnsafeMutablePointer and UnsafeReference to have the side-effect of allocating memory. All the others are relatively cheap transformations on pointer values, and get used a lot for typecasting. An allocating constructor would be less locatable among such uses of typecasting-via-constructor. The memory-allocating static method has the merit of sticking out, and pairs nicely with the necessary deallocation call, like the malloc/free pair.


(Ondrej Barina) #13

+1 for proposal
Ondrej

···

On Sat, Jan 23, 2016 at 9:59 AM, Jacob Bandes-Storch via swift-evolution < swift-evolution@swift.org> wrote:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.md

> What is your evaluation of the proposal?

I believe the Unmanaged -> UnsafeReference / OpaquePointer changes are too
large to be part of this proposal; they should be reviewed separately.
There was some discussion about this in a thread last month. Particularly,
I'm concerned that there is still a gap between UnsafePointer<Void> and
OpaquePointer APIs. I submitted a related proposal, SE-0017, which was
never scheduled for review:

https://github.com/apple/swift-evolution/blob/master/proposals/0017-convert-unmanaged-to-use-unsafepointer.md

I'm also a little concerned about Integer, SignedInteger, and
UnsignedInteger, which are protocols but might look like concrete integer
types to the reader. My first thought is that "Integral" would be better
for this, but it doesn't match the "noun or capability" note in the API
design guidelines.

Otherwise, I'm generally fine with the proposed changes.

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

Yes; consistency is important.

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

Yes, by definition.

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

Fairly in-depth.

Jacob

On Fri, Jan 22, 2016 at 1:02 PM, Douglas Gregor via swift-evolution < > swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0006 "Apply API Guidelines to the Standard Library"
begins now and runs through January 31, 2016. The proposal is available
here:

https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.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/0006-apply-api-guidelines-to-the-standard-library.md

Reply text

Other replies

<https://github.com/apple/swift-evolution#what-goes-into-a-review-1>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,

-Doug Gregor

Review Manager

_______________________________________________
swift-evolution mailing list
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


(Dave Abrahams) #14

That's an interesting point; thanks for raising it!

···

on Fri Jan 22 2016, Guillaume Lessard <swift-evolution@swift.org> wrote:

Hello,

I disagree with the following change to UnsafeMutablePointer:
- public static func alloc(num: Int) -> UnsafeMutablePointer<Pointee>
+ public init(allocatingCapacity count: Int)

This would make it the only constructor in any of OpaquePointer,
UnsafePointer, UnsafeMutablePointer and UnsafeReference to have the
side-effect of allocating memory. All the others are relatively cheap
transformations on pointer values, and get used a lot for
typecasting. An allocating constructor would be less locatable among
such uses of typecasting-via-constructor. The memory-allocating static
method has the merit of sticking out, and pairs nicely with the
necessary deallocation call, like the malloc/free pair.

--
-Dave


(Charles Kissinger) #15

I agree with all of the small criticisms mentioned below by Radoslaw except for the renaming of precondition() to require(). I think it is an improvement that it describes an action now, just like assert().

Count me among those who liked the ‘Type’ suffix for protocols though.

—CK

···

On Jan 25, 2016, at 7:40 AM, Radosław Pietruszewski via swift-evolution <swift-evolution@swift.org> wrote:

Hello all,

Just like with SE-0005 <https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160125/007658.html>, I’m overwhelmingly for this proposal. The Guidelines, as a whole do a great job at making Swift APIs more consistent and clearer, and applying them to the Swift stdlib is an important first step.

* * *

Nitpicks, criticisms and suggestions:

== precondition was renamed to require ==

This might be confusing to newcomers, as many languages use the word `require` as a keyword for what we'd call `import`. Although a bit more technical, `precondition` is unambiguous and still easily understandable. I feel like `required` does more damage than good.

== Removed Type from protocol names ==

Perhaps I’ve missed some discussion about this and I don’t see the context, but I’m not sure this is a positive change.

I fear this might be confusing in practice, at least in some contexts. For example, there's nothing signifying that "Boolean" or "Integer" are protocols and not actual types. Same with “Sequence”, “OptionSet”, etc. Perhaps it doesn't matter because everyone will naturally go for `Bool`, `Int`, and `Array` anyway. But I can imagine a lot of confusion if someone tried that anyway, or perhaps saw that in the autocompletion, or the standard library browser (with no intention of using the protocol).

I’m all for removing unnecessary noise and verbosity, but I think I would err on explicitness side here. It seemed like the -able/-Type convention did a good job disambiguating types you can actually instantiate from protocols, with very little “verbosity cost”.

== sort() => sorted(), sortInPlace() => sort() etc ==

I’m torn on this.

Frankly, I find both the “foo/fooInPlace” and “bar/barred” conventions awkward. Both seem weird. “InPlace” isn’t something I recall seeing anywhere else in API naming, and seems a bizarre way of signifying mutability. “-ed” doesn’t work with all words, so you sometimes have to go with “-ing”, or give up and cry. And then you have inconsistency that “-InPlace” doesn’t seem to have. Also, -ed/-ing can sometimes be difficult to write, especially for non-natives because of the “last letter is doubled” rule for some words.

But my biggest problem with this change is that IMHO we should encourage to use the transforming (non-mutating) variants by default. One way to achieve this as an API designer and slightly push people towards doing what’s considered best practice is to make the preferable variant easier to type. This might be a subtle change, but I think it matters. Before, if you really wanted to mutate something in place, you had to do that extra little bit of work typing “sortInPlace”, whereas what would be preferable most of the time had a simpler, shorter form: “sort” and would appear earlier in autocomplete.

== -ings in argument names ==

I’ve noticed these few tweaks in naming:

- mutating func removeAll(keepCapacity keepCapacity: Bool = false)
+ mutating func removeAll(keepingCapacity keepingCapacity: Bool = false)

public func transcode<...>(...
- stopOnError: Bool
+ stoppingOnError: Bool
) -> Bool

+ public init(allocatingCapacity count: Int)

I'm against this change. While I'm not fully convinced of the -ed/-ing rule for methods and properties, it does an important job by conveying the non-mutating semantics of a symbol described. In case of argument names, this rationale no longer applies.

The only reason to write "stoppingOnError" instead of "stopOnError" is to make method invocations sound more like real English sentences. This is the conventional Objective-C thinking the Guidelines largely step back from. In my opinion, this is futile and provides no readability benefits in this context. Method invocations are _not_ sentences. It's not English, it's code. And while making method names blatantly gramatically incorrect doesn't help readability, neither does forcing `-ing` endings to all boolean function arguments.

The only thing it does is it adds a few extra characters, an extra word ending the reader has to parse and understand. I know that it's a non-goal to make Swift code as terse as possible, and I'm not arguing for that. But the Guidelines seem to agree that adding extra verbosity _without a good reason_ is a bad thing. Because every extra word and symbol in code just adds to the cognitive load of the reader. And when it doesn't serve a purpose, it just decreases the signal-to-noise ratio.

Plus, as mentioned before, `-ed/-ing` can be tricky to spell for non-natives. This might not be a big deal, but given that this change provides no benefits, it's one more tiny thing you have to be careful not to get wrong when writing Swift.

And it's unnecessary:

   removeAll(keepCapacity: true)
   transcode(foo, bar, stopOnError: true)

Are just as clear and readable as:

   removeAll(keepingCapacity: true)
   transcode(foo, bar, stoppingOnError: true)

And the former isn't gramatically incorrect, because this isn't a sentence.

Apologies for nitpicking on this tiniest possible detail. I just care a lot that we don't create a precedent of trying to make everything sound like English unnecessarily and add verbosity bit by bit.

* * *

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?

Yes, and yes, with small details still worth reconsidering.

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

I’ve read the whole proposal, as well as the related proposals, and read the thread for this review.

Cross-linking to my SE-0005 review: https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160125/007658.html

Thanks,
— Radek

On 22 Jan 2016, at 22:02, Douglas Gregor via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Hello Swift community,

The review of SE-0006 "Apply API Guidelines to the Standard Library" begins now and runs through January 31, 2016. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0006-apply-api-guidelines-to-the-standard-library.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/0006-apply-api-guidelines-to-the-standard-library.md
Reply text

Other replies
<https://github.com/apple/swift-evolution#what-goes-into-a-review-1>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,

-Doug Gregor

Review Manager

_______________________________________________
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


(David Sweeris) #16

Same here… I like -Type for protocols that can only be used a generic constraint, and -able/-ible for protocols that can be “concrete” types.

···

On Jan 25, 2016, at 07:40, Radosław Pietruszewski via swift-evolution <swift-evolution@swift.org> wrote:

== Removed Type from protocol names ==

Perhaps I’ve missed some discussion about this and I don’t see the context, but I’m not sure this is a positive change.

I fear this might be confusing in practice, at least in some contexts. For example, there's nothing signifying that "Boolean" or "Integer" are protocols and not actual types. Same with “Sequence”, “OptionSet”, etc. Perhaps it doesn't matter because everyone will naturally go for `Bool`, `Int`, and `Array` anyway. But I can imagine a lot of confusion if someone tried that anyway, or perhaps saw that in the autocompletion, or the standard library browser (with no intention of using the protocol).

I’m all for removing unnecessary noise and verbosity, but I think I would err on explicitness side here. It seemed like the -able/-Type convention did a good job disambiguating types you can actually instantiate from protocols, with very little “verbosity cost”.


(Matt Whiteside) #17

I think this argument is outweighed by the benefit of making it easier to name things. I always found the decision about whether to suffix things with “-Type" to be a point of confusion, and I’m glad to see it being removed. As far as disambiguating instantiatable types from protocols, this seems like something that could be handled by the IDE, for example, it could put a strike-through line on the auto-complete options that you can’t use as types in the given context.

Matt

···

On Jan 25, 2016, at 07:40, Radosław Pietruszewski via swift-evolution <swift-evolution@swift.org> wrote:

It seemed like the -able/-Type convention did a good job disambiguating types you can actually instantiate from protocols, with very little “verbosity cost”.


(Brent Royal-Gordon) #18

What is the rationale for moving `unsafeUnwrap` into Optional but not `unsafeAddressOf` into AnyObject?

For one thing, because AnyObject has no methods and the compiler prevents it from being extended. Of course, as far as I can tell that limitation is unprincipled.

···

--
Brent Royal-Gordon
Architechies


(Dave Abrahams) #19

I'm generally in favor of the proposed changes. I'll just note some
minor points and disagreements:

* Like I mentioned in my SE-0023 review, I would be OK with keeping
the "Type" suffix for protocols but have no strong preference.

* I'm in favor of keeping `precondition()`. `require()` might be
easier to grasp at first but personally I really came to like
`precondition()`.
It fells both precise and I prefer the passive `precondition()` to the
active `require()` for this case. To me it fits the primary meaning
better; stating an API contract. The fact that the condition is
actively checked is secondary to that.

* I also agree with Radosław in that I prefer `removeAll(keepCapacity:
Bool)` to `removeAll(keepingCapacity: Bool)`.

Why?

I had a hard time justifying "keeping" to myself for a while, but
eventually I realized that this pattern is less ambiguous, at least in
general, since many verbs are also nouns. Okay, "keeps" haven't been
considered high-tech construction elements since the middle ages, but
it's easy to understand how you'd be interested in the capacity of a
keep.

* What is the rationale for moving `unsafeUnwrap` into Optional but
not `unsafeAddressOf` into AnyObject?

Language limitation: AnyObject can't be modified or extended.

I can certainly see the safety argument against moving it but I don't
see how that would apply to `unsafeAddressOf` but not `unsafeUnwrap`?

* `EnumeratedSequence` and `Repeated` feel weird to me. They make
sense given the API guidelines and the previous `EnumerateSequence`
and `Repeat` were a bit clunky as well but these somehow feel a bit
worse... That might be wholly subjective though and I don't really
have a good suggestion. The only thing that came to mind was
`EnumerationSequence` and `Repetition` but I'm not overly fond of
those either especially not to the point of deviating from the norm...

Yes, they're a little clunky. No, I don't have any better ideas either
:slight_smile:

* This is not a disagreement but I'd be interested in hearing the
reasons for replacing Generator(Type) with Iterator(Protocol) if
someone finds the time. I can speculate of course but it's probably
easier for someone to give me a short summary :slight_smile:

I think these messages give all the details:

http://news.gmane.org/find-root.php?message_id=m2h9i4gffx.fsf%40eno.apple.com
http://article.gmane.org/gmane.comp.lang.swift.evolution/5344

* Typo:

+ public func take() -> Memory // Should be Pointee

Nice, thanks.

···

on Mon Feb 01 2016, Janosch Hildebrand <swift-evolution@swift.org> wrote:

--
-Dave


(Radek Pietruszewski) #20

I'm generally in favor of the proposed changes. I'll just note some
minor points and disagreements:

* Like I mentioned in my SE-0023 review, I would be OK with keeping
the "Type" suffix for protocols but have no strong preference.

* I'm in favor of keeping `precondition()`. `require()` might be
easier to grasp at first but personally I really came to like
`precondition()`.
It fells both precise and I prefer the passive `precondition()` to the
active `require()` for this case. To me it fits the primary meaning
better; stating an API contract. The fact that the condition is
actively checked is secondary to that.

* I also agree with Radosław in that I prefer `removeAll(keepCapacity:
Bool)` to `removeAll(keepingCapacity: Bool)`.

Why?

I had a hard time justifying "keeping" to myself for a while, but
eventually I realized that this pattern is less ambiguous, at least in
general, since many verbs are also nouns. Okay, "keeps" haven't been
considered high-tech construction elements since the middle ages, but
it's easy to understand how you'd be interested in the capacity of a
keep.

Why not, though? adding `-ing`s in this context has all of the problems -ed/-ing has with method names, and none of the necessity of conveying mutability information.

What’s wrong with “keepCapacity” as a parameter name?

···

On 02 Feb 2016, at 02:41, Dave Abrahams via swift-evolution <swift-evolution@swift.org> wrote:
on Mon Feb 01 2016, Janosch Hildebrand <swift-evolution@swift.org> wrote:

* What is the rationale for moving `unsafeUnwrap` into Optional but
not `unsafeAddressOf` into AnyObject?

Language limitation: AnyObject can't be modified or extended.

I can certainly see the safety argument against moving it but I don't
see how that would apply to `unsafeAddressOf` but not `unsafeUnwrap`?

* `EnumeratedSequence` and `Repeated` feel weird to me. They make
sense given the API guidelines and the previous `EnumerateSequence`
and `Repeat` were a bit clunky as well but these somehow feel a bit
worse... That might be wholly subjective though and I don't really
have a good suggestion. The only thing that came to mind was
`EnumerationSequence` and `Repetition` but I'm not overly fond of
those either especially not to the point of deviating from the norm...

Yes, they're a little clunky. No, I don't have any better ideas either
:slight_smile:

* This is not a disagreement but I'd be interested in hearing the
reasons for replacing Generator(Type) with Iterator(Protocol) if
someone finds the time. I can speculate of course but it's probably
easier for someone to give me a short summary :slight_smile:

I think these messages give all the details:

http://news.gmane.org/find-root.php?message_id=m2h9i4gffx.fsf%40eno.apple.com
http://article.gmane.org/gmane.comp.lang.swift.evolution/5344

* Typo:

+ public func take() -> Memory // Should be Pointee

Nice, thanks.

--
-Dave

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