Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
loop #95
loop #95
Changes from 30 commits
080e9ee
3278f9f
e32090a
4a504d1
ddf4596
f220493
a228bc5
9300d08
b35e40f
f981278
b240d45
8509293
572d6d7
a0d67cc
1e67815
459cb87
ac83095
cdd08a9
03d0c69
7365703
e24c8c9
62439ec
d6fc42c
6eb6388
247f6d5
105374c
13e0f0d
6d17e63
e05581e
23c93b0
48e46f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The code from this line downward does not seem to be needed, since it doesn't check anything new.
A more appropriate test would probably be to set
options.currentLoop = expr
, then dostart, _ = items[1].__teal__(options)
and verifyoptions.breakBlocks
contains onlystart
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.
The above comment applies here too.
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.
After more thought I think the
start
andstep
expressions should also evaluate toTealType.none
, since we don't want them to add things to the stack. Could you add a check for those too?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 dont't think this line should be here?
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.
condStart.addIncoming(doStart)
seems unnecessary here, sincecompileTeal
invokesaddIncoming
on the start block of the entire AST.Also, can you use
setNextBlock()
instead of directly settingstartEnd.nextBlock
?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.
nit: some places refer to the doBlock as thenBranch, can you update these?
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 is a great test. Could you also 2 more versions of this which test break and continue? Hopefully it should be mostly copy-paste 😅