Skip to content

Implement revert#74

Merged
cburgdorf merged 3 commits intoargotorg:masterfrom
cburgdorf:christoph/feat/revert
Oct 9, 2020
Merged

Implement revert#74
cburgdorf merged 3 commits intoargotorg:masterfrom
cburgdorf:christoph/feat/revert

Conversation

@cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Oct 6, 2020

What was wrong?

We need to have support for revert expressions.

How was it fixed?

  1. Refactored test_function into two functions in order to re-use it for revert assertion
  2. Mapped revert to revert(0,0) in YUL (Ignoring revert reasons Implement revert reasons #75 for now)
  3. Added a contract test for revert
  4. Added coverage for revert in the spec

To-Do

  • Update Spec
  • Clean up commit history

@codecov-commenter
Copy link

Codecov Report

Merging #74 into master will decrease coverage by 0.12%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
- Coverage   88.02%   87.90%   -0.13%     
==========================================
  Files          36       36              
  Lines        2279     2281       +2     
==========================================
- Hits         2006     2005       -1     
- Misses        273      276       +3     
Impacted Files Coverage Δ
compiler/src/yul/mappers/functions.rs 0.00% <0.00%> (ø)
semantics/src/traversal/functions.rs 88.09% <0.00%> (-2.15%) ⬇️
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 c0a6a4f...35a7c61. Read the comment docs.

@cburgdorf cburgdorf mentioned this pull request Oct 6, 2020
@cburgdorf cburgdorf marked this pull request as draft October 6, 2020 15:49
@cburgdorf cburgdorf force-pushed the christoph/feat/revert branch from 35a7c61 to 0955a27 Compare October 7, 2020 12:55
@codecov-io
Copy link

codecov-io commented Oct 7, 2020

Codecov Report

Merging #74 into master will decrease coverage by 0.07%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
- Coverage   88.02%   87.94%   -0.08%     
==========================================
  Files          36       36              
  Lines        2279     2281       +2     
==========================================
  Hits         2006     2006              
- Misses        273      275       +2     
Impacted Files Coverage Δ
compiler/src/yul/mappers/functions.rs 0.00% <0.00%> (ø)
semantics/src/traversal/functions.rs 88.09% <0.00%> (-2.15%) ⬇️

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 fc0657f...635916e. Read the comment docs.

@cburgdorf cburgdorf force-pushed the christoph/feat/revert branch from 0955a27 to e9caee2 Compare October 7, 2020 12:58
@cburgdorf cburgdorf marked this pull request as ready for review October 7, 2020 13:11
@cburgdorf cburgdorf requested a review from g-r-a-n-t October 7, 2020 13:12
Copy link
Collaborator

@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.

Code looks good. Just a few things in the spec that should be improved, mainly that we're referring to revert as an expression when it is considered to be a statement.

@cburgdorf cburgdorf force-pushed the christoph/feat/revert branch from e9caee2 to d285eaa Compare October 8, 2020 14:00
@cburgdorf cburgdorf force-pushed the christoph/feat/revert branch from d285eaa to 635916e Compare October 8, 2020 14:06
@cburgdorf cburgdorf merged commit de034e0 into argotorg:master Oct 9, 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.

4 participants