Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stdlib] Change post-nil guarantee for IteratorProtocol.next() #1702

Merged

Conversation

PatrickPijnappel
Copy link
Contributor

What's in this pull request?

Swift evolution proposal: swiftlang/swift-evolution#213
Swift evolution thread: http://thread.gmane.org/gmane.comp.lang.swift.evolution/focus=8519
Update from #1544 to resolve conflicts with master.

Changes the guarantee for IteratorProtocol.next() to:

/// Advance to the next element and return it, or `nil` if no next element
/// exists.  Once `nil` has been returned, all subsequent calls return `nil`.
///
/// - Precondition: `next()` has not been applied to a copy of `self`
///   since the copy was made.

All IteratorType implementations in the standard library already comply.

@lattner
Copy link
Contributor

lattner commented May 4, 2016

SE-0052 is accepted, @dabrahams or @gribozavr, can you please review this? Thanks!

@lattner
Copy link
Contributor

lattner commented May 4, 2016

Oh, also @PatrickPijnappel it looks like this patch has conflicts. Can you please update it when you get a chance? Thanks!

@gribozavr
Copy link
Contributor

The wording LGTM, but I'd like @dabrahams to also take a look.

@PatrickPijnappel
Copy link
Contributor Author

PatrickPijnappel commented May 5, 2016

@lattner @gribozavr Ok, resolved the conflicts!

  • The first line of the docs in IteratorProtocol diverged a bit, but wasn't consistent with the docs for the implementations. I used the original formulation in this commit.
  • Also I standardized the docs on 'advances/returns' vs. the 'advance/return' which half of them used (as I understand it the former is the preferred style?).

@gribozavr
Copy link
Contributor

CC @natecook1000 for the documentation style.

@natecook1000
Copy link
Member

The documentation style ('advances' instead of 'advance') is right on. I think describing the nil-returning behavior in the IteratorProtocol documentation should be sufficient, though—does that note need to be on every iterator?

This paragraph could use a note about the behavior, too, since it's giving instructions about how to implement an iterator: https://github.com/apple/swift/pull/1702/files#diff-b9ba8d4ad76ba94c1eeabc504d582b57R120

@dabrahams
Copy link
Contributor

on Wed May 04 2016, Dmitri Gribenko <notifications-AT-github.aaakk.us.kg> wrote:

The wording LGTM, but I'd like @dabrahams to also take a look.

The summary should be a single sentence fragment; see
https://swift.org/documentation/api-design-guidelines/#write-doc-comment

Dave

@PatrickPijnappel
Copy link
Contributor Author

PatrickPijnappel commented May 6, 2016

@natecook1000 I'd say the nil behavior and the non-copy requirement are important for consumers of the API (is there a guideline on this?), though defintely I agree they should not be in the summary line. I'm out at the moment but will adjust on Monday!

@PatrickPijnappel
Copy link
Contributor Author

@dabrahams Ok moved the guarantee away from the summary, so that's now a single line!

@PatrickPijnappel
Copy link
Contributor Author

@natecook1000 @dabrahams Is there a guideline on what information in the protocol's doc comment is restated in the implementation's doc comment btw?

@dabrahams
Copy link
Contributor

No such guideline exists. BTW, I was not saying you should move that information out of the summary (nor was I saying you shouldn’t); if it belongs in the summary you can use a semicolon to join the parts instead of a period.

@PatrickPijnappel
Copy link
Contributor Author

@dabrahams Is there still something blocking this?

@gribozavr
Copy link
Contributor

@natecook1000 Could you review, please?

@natecook1000
Copy link
Member

@gribozavr LGTM

@gribozavr
Copy link
Contributor

@swift-ci Please test and merge

@PatrickPijnappel
Copy link
Contributor Author

@gribozavr Build seems to have failed on an unrelated issue.

@gribozavr
Copy link
Contributor

Indeed. What I want to test is that SourceKit tests (which sometimes include snippets of the doc comments) are not affected by this change.

@gribozavr
Copy link
Contributor

@swift-ci Please smoke test

@gribozavr
Copy link
Contributor

Tests have passed, merging!

@gribozavr gribozavr merged commit 4614adc into swiftlang:master May 30, 2016
@PatrickPijnappel PatrickPijnappel deleted the iterator-post-nil-guarantee branch May 30, 2016 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants