-
-
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
Implement no-copy subarray iteration (Implements #3386) #4584
Implement no-copy subarray iteration (Implements #3386) #4584
Conversation
# ```text | ||
# b -- c -- d -- | ||
# ``` | ||
def each(*, within range : Range(Int, Int)) |
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.
I don't really like requiring within:
. Even the example doesn't have it, so it's broken as far as I see 😛
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.
Thank you for checking, the example is broken. The start/count overload also use required named-arguments, I think (for consistency) changing the example should be enough
LGTM |
@cjgajard this looks good 👍 Are you around to fix the minor issues? |
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.
Why force named parameters?
They should at least be forced for start and count, and I don't mind the range being a forced named parameter too. I like the named params |
@RX14 A better question is why count is mandatory. Default could be to iterate to end. |
You could make it default to |
# 2 -- 3 -- | ||
# ``` | ||
def each_index(*, start : Int, count : Int) | ||
raise ArgumentError.new "negative count: #{count}" if count < 0 |
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.
Oops, Title cased Exception
messages, please.
…rystal-lang#4584) * Implement no-copy subarray iteration * Fix example for Indexable#each(*, within) * Fix Indexable#each_index(*, start, count) example
Since I deleted my previous fork I need to PR again.
This PR closes #3390.