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

Add zeroing function for arrays #1676

Merged
merged 18 commits into from
Apr 8, 2020

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Nov 1, 2019

What I did

How I did it

  • Get rid of NullType from codebase. Zeroing operations are now from the None sentinel value alone.
  • Factor mzero into a function

How to verify it

See tests

Description for the changelog

  • Introduce syntax for initializing new arrays with zero
  • Check array assignment is always between arrays of same length

Cute Animal Picture

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

@fubuloubu fubuloubu added the work in progress Work on this PR or issue is not yet complete but reviewers are free to add their input for guidance. label Nov 6, 2019
@charles-cooper charles-cooper changed the title WIP allow arrays to be syntactically uninitialized Add zeroing function for arrays Nov 12, 2019
@charles-cooper charles-cooper removed the work in progress Work on this PR or issue is not yet complete but reviewers are free to add their input for guidance. label Nov 12, 2019
@fubuloubu
Copy link
Member

fubuloubu commented Nov 12, 2019

Edit: moved discussion back to issue

@fubuloubu
Copy link
Member

can you rebase on master?

@charles-cooper
Copy link
Member Author

can you rebase on master?

@fubuloubu is this because you force pushed to master 😂 ? in the future can we protect master from all pushes except from PRs?

@fubuloubu
Copy link
Member

Maybe, that wasn't the commit that was lost though

vyper/parser/lll_node.py Outdated Show resolved Hide resolved
@charles-cooper charles-cooper added the work in progress Work on this PR or issue is not yet complete but reviewers are free to add their input for guidance. label Nov 28, 2019
@charles-cooper
Copy link
Member Author

some work on the gas estimator is still needed

@fubuloubu
Copy link
Member

Rebase on master to get updated CI

@charles-cooper charles-cooper force-pushed the uninitialized_array branch 2 times, most recently from ee5f52a to 37c8cbc Compare December 6, 2019 22:45
@charles-cooper
Copy link
Member Author

some work on the gas estimator is still needed

I fixed it.

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.

LGTM

Still want to discuss the naming though. I dislike clear

@fubuloubu
Copy link
Member

Meeting notes: Change to empty, leave current use of clear for now, but could be deprecated in the future.

Copy link
Contributor

@jacqueswww jacqueswww left a comment

Choose a reason for hiding this comment

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

Can we get this merged (see above renaming, then ✔️ ) ? 🚀

@michwill
Copy link

What would be pythonic is something like a: uint256[10] = [0 for _ in range(10)], or even a: uint256[10] = [0] * 10

@fubuloubu
Copy link
Member

@michwill the later would be the better choice, but we had a very long conversation about this and enabling the Pythonic list comprehension syntax is a rabbit hole, plus empty is more broadly useful.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 7, 2020

This pull request introduces 3 alerts when merging 47d07b8 into e27fd81 - view on LGTM.com

new alerts:

  • 2 for Syntax error
  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 7, 2020

This pull request introduces 2 alerts when merging 8fdb472 into e27fd81 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Unreachable code

this handles the transformation from None to 0 for base types.
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 7, 2020

This pull request introduces 3 alerts when merging dbb078a into e27fd81 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unreachable code

@codecov-io
Copy link

codecov-io commented Apr 7, 2020

Codecov Report

Merging #1676 into master will increase coverage by 0.12%.
The diff coverage is 85.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1676      +/-   ##
==========================================
+ Coverage   86.55%   86.67%   +0.12%     
==========================================
  Files          55       55              
  Lines        6343     6335       -8     
  Branches     1630     1629       -1     
==========================================
+ Hits         5490     5491       +1     
+ Misses        551      543       -8     
+ Partials      302      301       -1     
Impacted Files Coverage Δ
vyper/ast/nodes.py 85.50% <ø> (+0.85%) ⬆️
vyper/types/types.py 88.51% <ø> (-0.17%) ⬇️
vyper/parser/expr.py 87.96% <50.00%> (-0.40%) ⬇️
vyper/functions/functions.py 95.44% <60.00%> (-0.23%) ⬇️
vyper/parser/stmt.py 92.45% <83.33%> (+0.18%) ⬆️
vyper/parser/parser_utils.py 87.43% <88.88%> (+1.64%) ⬆️
vyper/parser/lll_node.py 82.93% <100.00%> (ø)
... and 1 more

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 e27fd81...865bcfd. Read the comment docs.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 7, 2020

This pull request introduces 1 alert when merging c9f935d into e27fd81 - view on LGTM.com

new alerts:

  • 1 for Unused import

)
def empty(expr, context):
if len(expr.args) != 1:
raise ParserException('function expects two parameters.', expr)
Copy link
Member

@fubuloubu fubuloubu Apr 7, 2020

Choose a reason for hiding this comment

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

Suggested change
raise ParserException('function expects two parameters.', expr)
raise ArgumentException('function expects one parameter.', expr)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think ArgumentException is the most fitting exception to raise here. ParserException implies that the code can't be parsed. It also doesn't support annotation.

vyper/parser/parser_utils.py Outdated Show resolved Hide resolved
@charles-cooper
Copy link
Member Author

I'm OK merging this with the caveat that users might have some trouble passing empty to and from functions. The compiler will probably just complain. Should be fixed once arg packing / unpacking is refactored.

@fubuloubu fubuloubu removed the work in progress Work on this PR or issue is not yet complete but reviewers are free to add their input for guidance. label Apr 8, 2020
)
def empty(expr, context):
if len(expr.args) != 1:
raise ParserException('function expects two parameters.', expr)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ArgumentException is the most fitting exception to raise here. ParserException implies that the code can't be parsed. It also doesn't support annotation.

vyper/parser/expr.py Outdated Show resolved Hide resolved
vyper/parser/parser_utils.py Outdated Show resolved Hide resolved
vyper/parser/stmt.py Outdated Show resolved Hide resolved
@fubuloubu fubuloubu merged commit 4514fa5 into vyperlang:master Apr 8, 2020
@or2008
Copy link

or2008 commented Jun 25, 2020

hello, how can I use empty to mimic solidity's new bytes(0)?

looks like empty(bytes[1024]) causes this exception:

vyper.exceptions.CompilerPanic: Unsupported location: None Please create an issue.vyper.exceptions.CompilerPanic: Unsupported location: None Please create an issue.

@fubuloubu
Copy link
Member

@or2008 can you do us a favor and create a new issue with the source code that is giving you the problem? Thanks!

@or2008
Copy link

or2008 commented Jul 17, 2020

sure, opened #2112

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.

initializing empty arrays
7 participants