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

Implement support for break statement #125

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

cburgdorf
Copy link
Collaborator

What was wrong?

We don't support break statements yet.

How was it fixed?

  1. Added a new BlockScopeType to BlockScope that can be one of Function, IfElse or Loop.
  2. Added pub fn inherits_type(&self, typ: BlockScopeType) -> bool to BlockScope to be able to tell if certain statements (e.g. break, continue) can be used within the current scope or not.
  3. Semantic pass checks with inherits_type if placement is ok and returns SemanticError::BreakWithoutLoop if not
  4. Mapper pass maps break to YUL

@codecov-io
Copy link

Codecov Report

Merging #125 (d279f2e) into master (05b6f3d) will increase coverage by 0.00%.
The diff coverage is 84.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #125   +/-   ##
=======================================
  Coverage   84.38%   84.39%           
=======================================
  Files          40       40           
  Lines        2556     2596   +40     
=======================================
+ Hits         2157     2191   +34     
- Misses        399      405    +6     
Impacted Files Coverage Δ
compiler/src/yul/mappers/functions.rs 0.00% <0.00%> (ø)
semantics/src/traversal/functions.rs 46.87% <0.00%> (-0.50%) ⬇️
semantics/src/namespace/scopes.rs 92.66% <93.33%> (-0.20%) ⬇️
compiler/src/abi/elements.rs 89.47% <0.00%> (-1.76%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05b6f3d...d279f2e. Read the comment docs.

Copy link
Member

@g-r-a-n-t g-r-a-n-t left a comment

Choose a reason for hiding this comment

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

👍

let block_scope_2 = BlockScope::from_block_scope(
Span::new(0, 0),
BlockScopeType::IfElse,
block_scope_1.clone(),
Copy link
Member

@g-r-a-n-t g-r-a-n-t Nov 17, 2020

Choose a reason for hiding this comment

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

I had been using Rc::clone(&scope), since that's what's used in the API docs for Rc: https://doc.rust-lang.org/std/rc/struct.Rc.html.

Both of these do the same thing, of course, and I don't have a strong opinion about which method we should use to call clone, but I think we should be consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for bringing this up. I wasn't aware that there was a good reason for the the more verbose syntax. I looked it up and RC::clone(&ptr) is indeed the preferred syntax. The reason for that is to make it clear what the clone is about since clone is a very vaguely defined action in Rust. In this case it is just increasing the reference counter whereas e.g. cloning a Vec<T> can cause quite a bit of memory getting copied.

References:

https://github.com/nical/rfcs/blob/rc-newref-clone/text/0000-rc-newref-clone.md
rust-lang/rust#42137 (comment)

I'm going to merge this as is and send a follow up PR to rewrite all occurrences with the idiomatic syntax.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated all usage in this PR #126

@cburgdorf cburgdorf merged commit c9e7352 into ethereum:master Nov 18, 2020
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