Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Makes AccountInfo repr(C) #30124

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Feb 3, 2023

Problem

Please see #29852.

Since AccountInfo does not contain any Vecs, we can move the fields around and add repr(C) to ensure its layout remains stable. (This assumes that the layouts for Rc,RefCell, and slice do not change, which is verified in the check_type_assumptions test.)

Summary of Changes

Add repr(C) to AccountInfo

@brooksprumo brooksprumo self-assigned this Feb 3, 2023
@brooksprumo brooksprumo marked this pull request as ready for review February 3, 2023 19:04
Copy link
Contributor

@alessandrod alessandrod left a comment

Choose a reason for hiding this comment

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

LGTM! I think it'd be nice to also reorder the checks in check_type_assumptions()? It checks the right offsets with the old field definition order which is a bit confusing

@brooksprumo
Copy link
Contributor Author

I think it'd be nice to also reorder the checks in check_type_assumptions()? It checks the right offsets with the old field definition order which is a bit confusing

Noted. I will address this in a subsequent PR.

@brooksprumo brooksprumo merged commit b77e12b into solana-labs:master Feb 6, 2023
@brooksprumo brooksprumo deleted the repr-c/account_info branch February 6, 2023 13:00
@brooksprumo
Copy link
Contributor Author

I think it'd be nice to also reorder the checks in check_type_assumptions()? It checks the right offsets with the old field definition order which is a bit confusing

Noted. I will address this in a subsequent PR.

#30143

joncinque pushed a commit to joncinque/solana that referenced this pull request Aug 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants