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

fix[codegen]: fix double eval of start in range expr #4033

Merged

Conversation

cyberthirst
Copy link
Collaborator

What I did

How I did it

  • cache the start argument to the stack

How to verify it

  • added a double eval test

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@cyberthirst cyberthirst added this to the v0.4.0 milestone May 18, 2024
@charles-cooper charles-cooper changed the title fix[codegen]: fix double eval of start in range fix[codegen]: fix double eval of start in range expr May 18, 2024
@charles-cooper
Copy link
Member

we should have tests for both 1 and 2 argument cases

@charles-cooper
Copy link
Member

also this seems fine, but didn't we just add a fence for pop()? this has a performance penalty as well.

@cyberthirst
Copy link
Collaborator Author

we should have tests for both 1 and 2 argument cases

addressed in: 8b5b364

Copy link

codecov bot commented May 25, 2024

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 52.70%. Comparing base (f0afaf0) to head (8b5b364).

Current head 8b5b364 differs from pull request most recent head d329685

Please upload reports for the commit d329685 to get more accurate results.

Files Patch % Lines
vyper/codegen/stmt.py 61.53% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4033       +/-   ##
===========================================
- Coverage   91.12%   52.70%   -38.42%     
===========================================
  Files         106      106               
  Lines       15308    15309        +1     
  Branches     3365     3366        +1     
===========================================
- Hits        13949     8069     -5880     
- Misses        925     6603     +5678     
- Partials      434      637      +203     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cyberthirst
Copy link
Collaborator Author

also this seems fine, but didn't we just add a fence for pop()? this has a performance penalty as well.

as discussed, it's a question of panicking for certain classes of contracts vs having some perf penalty

eg contracts like this wouldn't compile without the cache

@internal
@view
def bar() -> uint256:
    return 0

@external
def foo():
    for i:uint256 in range(self.bar(), 5, bound = 3):
        pass

@charles-cooper charles-cooper enabled auto-merge (squash) May 25, 2024 11:47
@charles-cooper charles-cooper merged commit b9244e8 into vyperlang:master May 25, 2024
155 checks passed
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.

2 participants