-
Notifications
You must be signed in to change notification settings - Fork 140
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
Add new syntax to bind for-in loop indices #1216
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1216 +/- ##
==========================================
+ Coverage 77.12% 77.14% +0.01%
==========================================
Files 272 272
Lines 33911 33944 +33
==========================================
+ Hits 26155 26186 +31
- Misses 6693 6694 +1
- Partials 1063 1064 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Nice work! 👍
BTW: We usually use a branch naming scheme like <name>/<issue-nunber>-<short-name>
to avoid conflicts, e.g. for this PR/branch daniel/402-for-loop-index
|
||
if indexVariable != nil { | ||
index++ | ||
indexValue.BigInt.SetInt64(index) |
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.
Is it safe to change the existing value, or should we assign a new value? I think changing the content of the existing value might also mutate the usages of previous indexes as well.
like for eg: what if a user stores the indexes in an array, and prints them later
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.
Ah, that's a good point. I had suggested this above to avoid allocation, but I think you are right
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 added a test for this, and there are is indeed an error in this case. Nice catch
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.
@dsainati1 Nice, good idea to add a test! Feel free to revert to your original code, that worked and I think was safe 👍
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.
Nice work! 👍
Closes #402
Description
This adds a new piece of syntax to for loops to the language to allow users to bind the index into the array being iterated over.
This introduces two variables
x
andy
into the scope of the loop body;y
is the element variable as in a normal for-in loop, whilex
is of typeint
, is initialized to 0 and is incremented by one at the end of each iteration of the loop, effectively tracking the index of the array being looped over.master
branchFiles changed
in the Github PR explorer