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

WIP: frontend for dynamic array functions #7

Merged

Conversation

tserg
Copy link

@tserg tserg commented Feb 2, 2022

What I did

Add 'append' and 'pop' to frontend

How I did it

  • Added a DynamicArrayFunctionDefinition class
  • Amend DynamicArrayDefinition to additionally inherit MemerTypeDefinition
  • Update semantics validation

How to verify it

Description for the changelog

Cute Animal Picture

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

@tserg
Copy link
Author

tserg commented Feb 2, 2022

Is this approach feasible?

@charles-cooper
Copy link
Owner

Is this approach feasible?

I think so actually. I would rename the DynArrayFunction class to MemberFunction or TypeMethod class (to distinguish from external contract calls) which is parametrized on type (i.e. takes DynArray as a constructor argument). This is cleaner because it's more generic over types and also opens up the door to adding methods to other types too.


if self.name == "append":
validation.utils.validate_expected_type(node.args[0], value_type)
return DynamicArrayDefinition(value_type, dynarray_length + 1)
Copy link
Owner

Choose a reason for hiding this comment

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

two things -- I think append() should return void, and also it won't change the length bound on the dynarray, it will only change the runtime length.

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2022

Codecov Report

Merging #7 (fb01036) into dyn_array_functions (a2dd0ca) will decrease coverage by 3.07%.
The diff coverage is 57.14%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           dyn_array_functions       #7      +/-   ##
=======================================================
- Coverage                66.71%   63.63%   -3.08%     
=======================================================
  Files                       92       92              
  Lines                     9481     9510      +29     
  Branches                  2371     2378       +7     
=======================================================
- Hits                      6325     6052     -273     
- Misses                    2460     2794     +334     
+ Partials                   696      664      -32     
Impacted Files Coverage Δ
vyper/semantics/validation/annotation.py 77.48% <42.85%> (-4.71%) ⬇️
vyper/semantics/validation/local.py 58.99% <50.00%> (ø)
vyper/semantics/types/function.py 62.54% <55.00%> (-0.79%) ⬇️
vyper/semantics/types/indexable/sequence.py 63.63% <83.33%> (+0.70%) ⬆️
vyper/builtin_functions/utils.py 0.00% <0.00%> (-100.00%) ⬇️
vyper/builtin_functions/functions.py 61.06% <0.00%> (-15.69%) ⬇️
vyper/exceptions.py 56.36% <0.00%> (-15.46%) ⬇️
vyper/codegen/expr.py 48.06% <0.00%> (-9.85%) ⬇️
vyper/ast/nodes.py 69.96% <0.00%> (-7.88%) ⬇️
vyper/semantics/types/value/numeric.py 80.95% <0.00%> (-4.77%) ⬇️
... and 6 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 a2dd0ca...fb01036. Read the comment docs.

@tserg
Copy link
Author

tserg commented Feb 3, 2022

Is this approach feasible?

I think so actually. I would rename the DynArrayFunction class to MemberFunction or TypeMethod class (to distinguish from external contract calls) which is parametrized on type (i.e. takes DynArray as a constructor argument). This is cleaner because it's more generic over types and also opens up the door to adding methods to other types too.

Cool, that is a good point. I opted for MemberFunction as I think its meaning would be more apparent for someone looking at the AST.

With this change, I think it would be more appropriate to move the MemberFunction class to function.py? It does however result in an circular import in DynamicArrayDefinition when adding the MemberFunction class as members (which I think should not be an issue).

Also, please let me know if there are any outstanding steps for the front-end, or if I can help on linking the front-end to the codegen.

@charles-cooper
Copy link
Owner

With this change, I think it would be more appropriate to move the MemberFunction class to function.py? It does however result in an circular import in DynamicArrayDefinition when adding the MemberFunction class as members (which I think should not be an issue).

yeah that move makes sense.

@tserg tserg marked this pull request as ready for review February 8, 2022 07:22
@charles-cooper charles-cooper merged commit 914eba8 into charles-cooper:dyn_array_functions Feb 10, 2022
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