Skip to content

Add InlineArrayType to support literals conversion to statically and dynamically allocated arrays. #12883

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

Closed
wants to merge 7 commits into from

Conversation

wechman
Copy link
Collaborator

@wechman wechman commented Mar 31, 2022

fixes #11879
fixes #13085

This is an early version of the changes needed to support literals conversion to dynamic arrays. Definitely it is not ready to review yet.

Things we should not forget to test:

  • assigning to storage
  • x = [x[1], x[0]], where x is a multi-dimensional storage array

@ekpyron
Copy link
Member

ekpyron commented Apr 6, 2022

Interestingly, at a quick glance, the optimizer seems to be handling this rather well even for larger-than-16-elements-arrays (without having to use stack-to-memory).

@ekpyron
Copy link
Member

ekpyron commented Apr 6, 2022

Random idea for a syntax test on this:

function f(uint256[] memory x) {}
function f(uint256[3] memory x) {}
function g() {
  f([1]); // should work
  f([1,2,3]); // should fail (no unique function)
  f([1,2,3,4]); // should work
}

This should work already, but good to have a test for it.

@wechman wechman force-pushed the array_literals_for_dynamic_arrays branch 2 times, most recently from 4d0b0c4 to d197426 Compare April 6, 2022 11:43
@wechman wechman force-pushed the array_literals_for_dynamic_arrays branch from 7347515 to 866a1d8 Compare April 8, 2022 05:54
@wechman wechman force-pushed the array_literals_for_dynamic_arrays branch from 127548a to 04c988f Compare April 8, 2022 11:40
@chriseth
Copy link
Contributor

Please note that I think it makes sense to re-unify this with tuple type again. For now, it helps to find places where it is used as an inline array, but in the end, they types are very similar, especially when we keep just putting things on the stack.


Type const* InlineArrayType::mobileType() const
{
solUnimplemented("Decide it it has a mobile type");
Copy link
Member

Choose a reason for hiding this comment

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

Given the weird index access cases like [1,2,3][idx] = 42; and return [1,2,3][idx];, it may in the end turn out that it makes sense to assign the statically sized array of the common type of the mobile types of all component types as a mobile type here :-).
Then these index accesses could just convert the inline array to its mobile type...
However, I'm not sure what other implications setting a proper mobile type here would have, so this needs some care...

@wechman wechman force-pushed the array_literals_for_dynamic_arrays branch from 5b64bf7 to 1c14212 Compare April 12, 2022 05:03
@wechman wechman force-pushed the array_literals_for_dynamic_arrays branch 6 times, most recently from 1c87ab1 to f937706 Compare April 22, 2022 04:41
@ethereum ethereum deleted a comment from stackenbotten Apr 22, 2022
@ethereum ethereum deleted a comment from stackenbotten Apr 22, 2022
@ethereum ethereum deleted a comment from stackenbotten Apr 22, 2022
@ethereum ethereum deleted a comment from stackenbotten Apr 22, 2022
@wechman wechman dismissed a stale review via a2b641c August 11, 2022 09:37
@wechman wechman force-pushed the array_literals_for_dynamic_arrays branch 2 times, most recently from a2b641c to 51ef84c Compare August 11, 2022 09:47
@wechman wechman requested a review from chriseth August 12, 2022 09:11
@wechman wechman force-pushed the array_literals_for_dynamic_arrays branch from 5f8cfce to 08e4d21 Compare August 24, 2022 12:39
@wechman wechman force-pushed the array_literals_for_dynamic_arrays branch from 08e4d21 to bb40435 Compare August 29, 2022 05:59
@cameel cameel added the takeover Can be taken over by someone other than label giver label Oct 25, 2022
@github-actions
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Nov 17, 2022
@github-actions
Copy link

This pull request was closed due to a lack of activity for 7 days after it was stale.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Nov 24, 2022
@github-actions github-actions bot closed this Nov 24, 2022
@cameel cameel removed closed due inactivity The issue/PR was automatically closed due to inactivity. stale The issue/PR was marked as stale because it has been open for too long. labels Nov 24, 2022
@cameel cameel reopened this Nov 24, 2022
@github-actions
Copy link

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

@ekpyron ekpyron marked this pull request as draft December 1, 2022 14:40
@cameel cameel removed the selected for development It's on our short-term development label Dec 2, 2022
@NunoFilipeSantos
Copy link
Contributor

Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
takeover Can be taken over by someone other than label giver
Projects
Archived in project
8 participants