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

Allow uint256 as iterator type in range-based for loop #2180

Merged

Conversation

iamdefinitelyahuman
Copy link
Contributor

@iamdefinitelyahuman iamdefinitelyahuman commented Oct 5, 2020

What I did

Allow use of types other than int128 for range-based for loops.

Fixes #2153
Fixes #2185

How I did it

This was already made possible in the type checking pass, but it gets blocked in parser.stmt. The proper solution is to refactor parser, but that is still potentially months away. As a stopgap solution:

  • attach type information to the iterator target AST node during type checking.
  • when generating LLL, use the given type instead of assuming int128.
  • if no type information is attached (happens when parser generates smaller ASTs later in the compilation phase, without any type checking), default to int128

How to verify it

Run tests.

Cute Animal Picture

image

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2020

Codecov Report

Merging #2180 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2180   +/-   ##
=======================================
  Coverage   85.32%   85.33%           
=======================================
  Files          83       83           
  Lines        8403     8405    +2     
  Branches     2032     2032           
=======================================
+ Hits         7170     7172    +2     
  Misses        731      731           
  Partials      502      502           
Impacted Files Coverage Δ
vyper/ast/nodes.py 93.84% <100.00%> (ø)
vyper/context/validation/local.py 88.42% <100.00%> (+0.04%) ⬆️
vyper/parser/stmt.py 79.79% <100.00%> (+0.06%) ⬆️

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 a0c561c...ca1f810. Read the comment docs.

Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Add a few tests and g2g

@codecov-io
Copy link

codecov-io commented Oct 9, 2020

Codecov Report

Merging #2180 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2180      +/-   ##
==========================================
- Coverage   85.32%   85.28%   -0.05%     
==========================================
  Files          83       79       -4     
  Lines        8403     8221     -182     
  Branches     2032     2005      -27     
==========================================
- Hits         7170     7011     -159     
+ Misses        731      711      -20     
+ Partials      502      499       -3     
Impacted Files Coverage Δ
vyper/ast/nodes.py 93.84% <100.00%> (ø)
vyper/context/validation/local.py 90.08% <100.00%> (+1.70%) ⬆️
vyper/parser/stmt.py 79.79% <100.00%> (+0.06%) ⬆️
vyper/parser/parser_utils.py 80.49% <0.00%> (-0.16%) ⬇️
vyper/opcodes.py
vyper/exceptions.py
vyper/typing.py
vyper/__main__.py

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 a0c561c...7f3815b. Read the comment docs.

@fubuloubu
Copy link
Member

fubuloubu commented Oct 9, 2020

Why'd coverage go down so much here?

EDIT: premature update from codecov

@iamdefinitelyahuman
Copy link
Contributor Author

There is a very minor loss to coverage in vyper/parser/parser_utils.py. I think it's that failing paths are no longer being hit, since i is no longer required to be int128.

@fubuloubu fubuloubu merged commit ce85d7d into vyperlang:master Oct 9, 2020
@iamdefinitelyahuman iamdefinitelyahuman deleted the fix-for-iterator-type branch October 9, 2020 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants