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

[WIP] auto generate public values structure and index #357

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hero78119
Copy link
Collaborator

@hero78119 hero78119 commented Oct 11, 2024

This PR resolve possible tech debt in the future which defines public value index against the struct in different place, which might lead to potential mismatch.

A getter/setter/index are all generate via rust macro

Copy link
Collaborator

@matthiasgoergens matthiasgoergens left a comment

Choose a reason for hiding this comment

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

I like what you are trying to do, but there's much simpler ways to achieve the same result.

You can use repr(C) to make sure that your struct and slice have the same memory layout. See #475 for how that can look like.

That way you don't need getters and setters, but can use a struct (and even nested structs) with named fields that rust-analyzer etc understand.

@lispc
Copy link
Collaborator

lispc commented Nov 28, 2024

i feel this is outdated and can be closed?

@lispc
Copy link
Collaborator

lispc commented Nov 28, 2024

repr(C)

i usually do not do this in our use cases. This is like mixing hardware encoding and business encoding. It is like we don't use protobuf for eth protocol wire encoding.

repr(C) like idea relates to programming language, compiler, cpu, os. (for example, i remember C or C++ support non-8-bit-byte?). Fully writing your encoding inside codes explicitly means fully portable readable stable.

@hero78119
Copy link
Collaborator Author

hero78119 commented Nov 28, 2024

I will make it as draft first (need to resolve conflict), for this PR still matter to address possible future bug, just not very urgent :)

After we are clear for how to set public input, then we can go back to review this PR

@hero78119 hero78119 marked this pull request as draft November 28, 2024 06:04
@hero78119 hero78119 changed the title auto generate public values structure and index [WIP] auto generate public values structure and index Nov 28, 2024
@matthiasgoergens
Copy link
Collaborator

matthiasgoergens commented Nov 28, 2024

repr(C)

i usually do not do this in our use cases. This is like mixing hardware encoding and business encoding. It is like we don't use protobuf for eth protocol wire encoding.

repr(C) like idea relates to programming language, compiler, cpu, os. (for example, i remember C or C++ support non-8-bit-byte?). Fully writing your encoding inside codes explicitly means fully portable readable stable.

In principle you would be right, but in Rust repr(C) is used a bit more generically than just for C compatibility. So the name is a bit misleading. It doesn't really necessarily have that much to do with C (and nothing with C++).

See the docs

[repr(C)] is also necessary to soundly do more elaborate tricks with data layout such as reinterpreting values as a different type.

In any case, #475 is a better implementation of a similar idea. And without the Java-insipired getter-setter-hell. We can port the technique to other structs instead of InsnRecord, if we don't want it for that specific one. (I can do that quickly.)

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