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

Fix recursive struct issue #1522

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

LucasSte
Copy link
Contributor

@LucasSte LucasSte commented Sep 13, 2023

Structs that contains themselves in a dynamic array are not recursive and do not have infinite size, as in the example below:

struct C {
    C [2][3][][2] m;
}

@LucasSte
Copy link
Contributor Author

@xermicus I implemented the fix for Polkadot as well, but I could not write tests for it because I've hit a problem in ABI generation (#1520). Can you advise if I should maintain the changes I made in Polkadot or revert them?

@LucasSte LucasSte marked this pull request as ready for review September 14, 2023 13:50
src/sema/types.rs Outdated Show resolved Hide resolved
src/sema/types.rs Outdated Show resolved Hide resolved
src/sema/types.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@seanyoung seanyoung left a comment

Choose a reason for hiding this comment

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

LGTM!

src/sema/types.rs Outdated Show resolved Hide resolved
src/sema/types.rs Outdated Show resolved Hide resolved
src/sema/types.rs Outdated Show resolved Hide resolved
src/sema/types.rs Outdated Show resolved Hide resolved
src/sema/types.rs Outdated Show resolved Hide resolved
Type::Array(_, dims) if dims.contains(&ArrayLength::Dynamic) => {
let size = dynamic_array_size(dims);
// A pointer is four bytes on Solana
size * 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be the size + 4 times the number of dynamic arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand, an array uint8 [2][][3] means a fixed-length array of pointers that contains three elements. Each pointer points to uint8[2]. The size will only be 3*pointer_size. Now if the type was uint8 [2][][][3], we'll still have a fixed-array of pointers that contains three elements. Each pointer points to a uint8[2][]. The size will continue to be 3*pointer_size.

The function calculates the minimum size we need for storage. As soon as we allocate elements, more space will be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks.

What you explained here would look good in a comment to the source code (or even in the doc comment of this function)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is explained right above dynamic_array_size between lines 2172 and 2176.

Copy link
Contributor

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

There ar so many issues with struct

Type::Array(_, dims) if dims.contains(&ArrayLength::Dynamic) => {
let size = dynamic_array_size(dims);
// A pointer is four bytes on Solana
size * 4
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks.

What you explained here would look good in a comment to the source code (or even in the doc comment of this function)

@LucasSte LucasSte requested a review from xermicus September 19, 2023 20:32
Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>
@LucasSte LucasSte merged commit c2d0fd8 into hyperledger-solang:main Sep 21, 2023
16 checks passed
@LucasSte LucasSte deleted the recursive-struct branch September 21, 2023 19:05
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