-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Update iterator.dart to fully spec moveNext() #39676
Conversation
The `moveNext` docs should contain all the information necessary to implement it. There is otherwise no mention that this method is also used to set `current` to the initial element.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Code review in https://dart-review.googlesource.com/c/sdk/+/127455 |
@@ -39,6 +39,9 @@ abstract class Iterator<E> { | |||
* Returns true if [current] contains the next element. | |||
* Returns false if no elements are left. | |||
* | |||
* Invoke [moveNext] to move to the first element and to each | |||
* subsequent element. | |||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repeats the behavior mentioned in the class description, but it does make it easier to find this information while only having an Iterator
typed variable available.
I'd prefer to phrase it slightly differently (we rarely use "invoke" in documentation, it's too formal). Maybe:
Use the [moveNext] method to move to the next element. This will change the
current value to that of the next element if there is a next element, which is when
[moveNext] returnstrue
. Also use [moveNext] to start the iteration before accessing
the first element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I misread this as a comment on current
.
I'm not sure it's suitable as an addition on moveNext
, because the documentation does say that already, but the existing text could probably be rewritten to express the same thing more clearly.
Maybe:
/** * Advances the iterator to the next element of the iteration. * * Should be called before reading [current]. It the call to `moveNext` returns * `true`, then [current] will contain the next element of the iteration until * `moveNext` is called again. If the call returns `false`, there are no further * elements and [current] should not be used any more. * * It is safe to call [moveNext] after it has already returned `false`, * but it must keep returning `false` and not have any other effect. * * A call to `moveNext` may throw for various reasons, including a concurrent change * to an underlying collection. If that happens, the iterator may be in an inconsistent * state, and any further behavior of the iterator is unspecified, * including the effect of reading [current]. */
How does this read to you?
@jtlapp thanks for your contribution, and sorry the delay. Please see review feedback above. |
I'm having trouble parsing your proposed alternatives. They aren't clear English. The |
If you have trouble parsing it, then it's probably not good documentation :). The problem with describing what So, maybe:
|
That looks fine to me. You put more care into the explanation than I did. |
Sync with content agreed to in PR discussion
The
moveNext
docs should contain all the information necessary to implement it. There is otherwise no mention that this method is also used to setcurrent
to the initial element.