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

Craetes a function to get the max size for abi.Argumetns, given any outermost slices have N elements #11259

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

nolag
Copy link
Contributor

@nolag nolag commented Nov 10, 2023

No description provided.

Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@nolag nolag force-pushed the rtinianov_parse_types branch from cc85045 to 584424a Compare November 15, 2023 16:14
@nolag nolag requested review from a team, jmank88 and krehermann November 15, 2023 16:14
@nolag nolag force-pushed the rtinianov_parse_types branch from 584424a to 985619a Compare November 15, 2023 16:26
@nolag nolag force-pushed the rtinianov_sizes branch 2 times, most recently from 38a06f9 to 17bd691 Compare November 15, 2023 16:49
@nolag nolag force-pushed the rtinianov_parse_types branch from 985619a to 563b404 Compare November 15, 2023 16:53
@nolag nolag force-pushed the rtinianov_parse_types branch from 563b404 to 917ecec Compare November 15, 2023 18:30
@nolag nolag force-pushed the rtinianov_parse_types branch 2 times, most recently from 0389024 to cd92e54 Compare November 27, 2023 19:11
@nolag nolag requested review from a team, bolekk, justinkaseman and KuphJr November 27, 2023 19:11
@nolag nolag force-pushed the rtinianov_parse_types branch from cd92e54 to 17fe123 Compare November 27, 2023 20:22
Base automatically changed from rtinianov_parse_types to BCF-2612-ChainReader-Next November 28, 2023 14:44
return size, nil
}

func getTypeSize(n int, t *abi.Type, dynamicTypeAllowed bool, isNested bool) (int, bool, error) {
Copy link
Contributor

@ilija42 ilija42 Nov 28, 2023

Choose a reason for hiding this comment

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

nit, why is abi.Type a pointer? Can't we just make this not a ptr and then not use tmp var in GetMaxSize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's often returned as one, so I figured why bother having the struct copied by value

Copy link
Contributor

@ilija42 ilija42 Nov 28, 2023

Choose a reason for hiding this comment

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

wdym, why would you copy it by value? If you pass a dereferenced arg.Type into getTypeSize() you get the same effect as if you passed &tmp in. Non pointer attributes inside of the struct won't get changed and attributes with a pointer would get changed either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See below, t.Elm is passed in on line 27, it's naturally a pointer. When you pass a struct directly, you force a copy of that struct to be made.

@nolag nolag requested review from jmank88 and ilija42 November 28, 2023 19:30
@cl-sonarqube-production
Copy link

@nolag nolag merged commit dab9762 into BCF-2612-ChainReader-Next Nov 28, 2023
85 checks passed
@nolag nolag deleted the rtinianov_sizes branch November 28, 2023 20:47
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