-
-
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 loop #5975
Update loop #5975
Conversation
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.
Please make the commit summary more descriptive.
Note: I am not voting for or against this change, just checking implementation.
src/kernel.cr
Outdated
@@ -25,8 +25,7 @@ ARGF = IO::ARGF.new(ARGV, STDIN) | |||
# # ... | |||
# end | |||
# ``` | |||
def loop | |||
i = 0 | |||
def loop(i = 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.
This should have a better name, like start
.
Could utilize external names
This adds an optional argument to the top level `loop` method, while continuing to default to `0`.
Even though this is correct, maybe it's time to remove the argument to loop. It's not intuitive, it's Int32 by default, and it can overflow. I'd vote to remove it. |
If
|
Yes, that's what you had to do before. Well, almost, you'd also increment the counter ;-) |
Oh yes, updating the counter inside the loop 😄 |
I love the counter in loop, make it Int128 and were all OK :) |
@asterite I'd rather add a |
Imagine one day we raise on overflow. Then |
Again, we can add a note. Someone doesn't read it and they have a loop in a server: loop do
# accept connection
end if we ever introduce raise on overflow, that will eventually raise. Why not make it explicit and use a counter, instead of |
For iterating other indices |
Could we follow the example of |
We can't iterate infinitely over a Range, though |
Ref #1407 |
I think it would be fine to keep the iteration, especially since it gets optimized out when unused. If we ever introduce a raise on overflow, it would be easy to provide backwards compatibility by having the definition of |
Can we do loop do
# no counter used
end loop do |c|
puts c
end |
Yes, that's what happens currently. If you compile with the |
I can't really see any way to make this counter make sense after we add overflow detection, so I'm fine for removing it. |
Without a counter, there is however little benefit for |
@straight-shoota the block scope is handy, if you want work being done inside the loop but don't want the work to leak outside |
@straight-shoota I use |
This adds an optional argument to the top level
loop
method. Sometimes it makes sense to start iterating at a different number than 0.