Skip to content
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

Validate missing else branch #417

Merged
merged 2 commits into from
Jul 27, 2020
Merged

Validate missing else branch #417

merged 2 commits into from
Jul 27, 2020

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Jul 13, 2020

Depends on #407

@codecov
Copy link

codecov bot commented Jul 13, 2020

Codecov Report

Merging #417 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
+ Coverage   99.32%   99.33%   +0.01%     
==========================================
  Files          50       50              
  Lines       14075    14169      +94     
==========================================
+ Hits        13980    14075      +95     
+ Misses         95       94       -1     

@gumb0 gumb0 force-pushed the validate-missing-else branch from 9ef06c0 to 0ef2d6d Compare July 13, 2020 11:27
@gumb0 gumb0 force-pushed the validate-types-block branch 2 times, most recently from e8db0dc to 98ae70c Compare July 17, 2020 15:17
Base automatically changed from validate-types-block to master July 20, 2020 09:39
@axic
Copy link
Member

axic commented Jul 20, 2020

@gumb0 rebase plase

@gumb0 gumb0 force-pushed the validate-missing-else branch from 0ef2d6d to 018b7bf Compare July 20, 2020 10:40
@@ -468,10 +467,15 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, FuncIdx f

update_result_stack(frame, operand_stack); // else is the end of if.

// Reset frame after if.
frame.unreachable = false;
Copy link
Collaborator Author

@gumb0 gumb0 Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided here at else instruction to pop the frame and push a new one instead of overwriting fields in the top one.

This way we can keep both instruction and immediates_offset fields const in ControlFrame, and it just seems more logical to push a new one, because it's different frame.

But this requires to transfer the contents of br_immediate_offsets from the old top frame to the new one.

@gumb0 gumb0 force-pushed the validate-missing-else branch from 018b7bf to b65b462 Compare July 20, 2020 10:46
@gumb0 gumb0 requested review from chfast and axic July 20, 2020 10:48
Copy link
Collaborator

@chfast chfast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach. Something more sophisticated may be to create a new frame and then swap with the old one. And also swap br offsets vector. Just a way to hide double move. Not verified. Just napkin level idea.

Copy link
Collaborator

@chfast chfast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach. Something more sophisticated may be to create a new frame and then swap with the old one. And also swap br offsets vector. Just a way to hide double move. Not verified. Just napkin level idea.

@gumb0 gumb0 force-pushed the validate-missing-else branch from b65b462 to 15eb164 Compare July 27, 2020 10:46
@gumb0 gumb0 merged commit dbf56f9 into master Jul 27, 2020
@gumb0 gumb0 deleted the validate-missing-else branch July 27, 2020 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants