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

feat: add extend for dynamic arrays #2976

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Jul 19, 2022

What I did

Add extend() for dynamic arrays, partial fix for #2611.

The source dynamic array (argument) must be of dynamic array type, up to the size of the destination dynamic array.

How I did it

  • Add extend_dyn_array helper to codegen/core.py.
  • Add extend as member function of dynamic array type.

How to verify it

See tests

Commit message

feat: add extend for dynamic arrays

Description for the changelog

Add extend() for dynamic arrays.

Cute Animal Picture

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

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2022

Codecov Report

Merging #2976 (96c680e) into master (c71b023) will increase coverage by 0.01%.
The diff coverage is 97.08%.

@@            Coverage Diff             @@
##           master    #2976      +/-   ##
==========================================
+ Coverage   88.35%   88.37%   +0.01%     
==========================================
  Files          97       97              
  Lines       10968    11003      +35     
  Branches     2593     2599       +6     
==========================================
+ Hits         9691     9724      +33     
- Misses        827      828       +1     
- Partials      450      451       +1     
Impacted Files Coverage Δ
vyper/codegen/core.py 84.26% <91.42%> (-0.93%) ⬇️
vyper/builtin_functions/functions.py 90.82% <100.00%> (+0.48%) ⬆️
vyper/codegen/expr.py 85.12% <100.00%> (-0.11%) ⬇️
vyper/codegen/stmt.py 87.76% <100.00%> (-0.40%) ⬇️
vyper/semantics/types/indexable/sequence.py 88.02% <100.00%> (+0.08%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tserg tserg marked this pull request as ready for review July 21, 2022 12:47
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.

This is looking pretty good

return b1.resolve(b2.resolve(ret))


def _copy_dynarray_body(dst, src, loop_count=None):
Copy link
Member

Choose a reason for hiding this comment

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

is loop_count a necessary parameter for this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we do not pass in the loop_count from _dynarray_make_setter, the test_nested_struct_of_lists test in tests/parser/types/test_dynamic_array.py will pass with the --no-optimize flag instead of throwing a compilation error. Should I amend the test?

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

this PR is shaping up, left a couple nits. thinking about whether we might want to move the dynarray method implementations into the builtins file as well? that would probably require exposing copy_dynarray_body as a public method of codegen.core though.

@tserg
Copy link
Collaborator Author

tserg commented Jul 27, 2022

this PR is shaping up, left a couple nits. thinking about whether we might want to move the dynarray method implementations into the builtins file as well? that would probably require exposing copy_dynarray_body as a public method of codegen.core though.

Cool, I think that would be neater since they are used only in vyper/builtin_functions/functions.py. Should I make the change?

dst_start_idx = get_element_ptr(dst, dst_len, array_bounds_check=False)

# Cast dst start pointer as darray for `_dynarray_make_setter` by subtracting offset
dst_i = IRnode.from_list(["sub", dst_start_idx, dst.location.word_scale])
Copy link
Member

Choose a reason for hiding this comment

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

kind of bothers me that we still have to adjust the offset like this for copy_dynarray_body

@tserg tserg requested a review from charles-cooper August 11, 2022 13:30
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