Skip to content

Don't erase Box types #82

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Don't erase Box types #82

wants to merge 8 commits into from

Conversation

ascjones
Copy link
Contributor

For rust type generators we need to know if a type is a Box in order to generate e.g. valid non self-referential types. This can be done either by defining a custom composite type as I have done here, or maybe as a special case TypeDef enum variant.

@dvdplm
Copy link
Contributor

dvdplm commented Apr 12, 2021

Do you think we should do the same for other smart pointer types, Rc<T>, Arc<T>, Ref<T>, RefMut<T> etc? Or wait until we know we need them?

@Robbepop
Copy link
Contributor

Do you think we should do the same for other smart pointer types, Rc<T>, Arc<T>, Ref<T>, RefMut<T> etc? Or wait until we know we need them?

We definitely do need them to be properly serialized. This is where there might be different use cases though since it won't make a lot of sense to do this in other contexts such as Javascript UIs.

@ascjones
Copy link
Contributor Author

ascjones commented Apr 12, 2021

Maybe we could use the WrapperTypeEncode concept from codec to encapsulate these kind of types, capturing the name of the wrapper type along with the wrapped type. https://github.com/paritytech/parity-scale-codec/blob/ab18de2477cc2a3421cd647092ebfe76b362cbd7/src/codec.rs#L344

@dvdplm
Copy link
Contributor

dvdplm commented Apr 13, 2021

I think we should wait until we need them so we have a clear use case in front of us.

@ascjones ascjones mentioned this pull request May 6, 2021
9 tasks
@ascjones ascjones marked this pull request as ready for review June 6, 2021 16:39
@ascjones
Copy link
Contributor Author

ascjones commented Jun 6, 2021

Updated and marked ready for review. I'm happy with this solution for Box on its own for now, can consider other smart pointers if and when we need them.

@Robbepop
Copy link
Contributor

Robbepop commented Jun 7, 2021

I think we want a clearly defined enum variant for this kind of type.
This type should be called Indirection since that is what it actually is if we abstract from Rust.
The type is not stored inline but indirectly or out-of-line to the object which it owns.
This also simplifies the interaction with UIs with this new Indirection variant because most UIs only care about the serialization and deserialization information anyways.

@Robbepop
Copy link
Contributor

What do you think about a middle ground solution where we introduce this Indirection type definition on the one hand side but still keep proper custom types for all of Box, Rc, Arc etc.? This way we could quickly identify indirectional types and would not have the problem of having to be too precise about them, e.g. introducing a RefCountedIndirection for Rc.

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