[Sol->Yul] Implement uint256[] memory arrays #7015
Conversation
| } | ||
|
|
||
| return Whiskers(R"( | ||
| function <functionName>(baseRef, index) -> addr { |
There was a problem hiding this comment.
It might be slightly more complicated than that, maybe do it later.
Marenz
force-pushed
the
sol-yul-arrays
branch
2 times, most recently
from
0a91de5 to
4f7303f
Compare
|
This PR is ready for review/merge |
Marenz
force-pushed
the
sol-yul-arrays
branch
2 times, most recently
from
69ab5e6 to
0dce871
Compare
| } | ||
| )") | ||
| ("functionName", functionName) | ||
| ("combine", combineExternalFunctionIdFunction()) |
There was a problem hiding this comment.
Does this perform cleanup?
There was a problem hiding this comment.
No, but I think we said we leave that for now as there was no function for cleaning up addr/selector. The evm code was only cleaning up addr I think and it used FunctionType for it..
There was a problem hiding this comment.
Ah, the function actually already performs cleanup.
| @@ -17,10 +17,6 @@ contract C { | |||
|
|
|||
| return func() == internal_func(); | |||
| } | |||
| function external_func() external pure returns (int8) | |||
There was a problem hiding this comment.
It was useless and wasn't even called.
|
Can you please add a test where a function creates a memory array and passes it via parameter to an internal function? |
|
|
||
| string prepared = _value; | ||
|
|
||
| // Exists to see if this case ever happens |
There was a problem hiding this comment.
Wouldn't it happen for implicit conversion?
There was a problem hiding this comment.
I think the conversion is already requested earlier.
| )") | ||
| ("functionName", functionName) | ||
| ("arrayLen", arrayLengthFunction(_type)) | ||
| ("headSize", to_string(_type.memoryHeadSize())) |
There was a problem hiding this comment.
Is this correct? Isn't the idea to multiply the index by the size of each element? If so I would imagine this should use the base type.
In case the above is true, using ArrayType::memoryStride() would allow removing <?noByteArray>.
There was a problem hiding this comment.
Yep! #7015 (review)
@Marenz why didn't you include that change suggestion?
There was a problem hiding this comment.
I made the change locally but haven't pushed yet. I mark things as resolved as I fix them and then push.
There was a problem hiding this comment.
Changing it to use memorystride
There was a problem hiding this comment.
Ah, indeed, that seems like the perfect fit ;)
|
Updated |
|
Test is failing. |
|
That's what I get for not recompiling before pushing |
|
pushed fixed |
Part of #8343.