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

Replace type aliases with newtypes #76

Open
repnop opened this issue Mar 4, 2022 · 3 comments
Open

Replace type aliases with newtypes #76

repnop opened this issue Mar 4, 2022 · 3 comments
Labels
area/types Related to rasn’s types for ASN.1 help wanted Extra attention is needed kind/enhancement New feature or request

Comments

@repnop
Copy link
Contributor

repnop commented Mar 4, 2022

Instead of directly reexporting types such as BigInt as Integer and Bytes as OctetString, I think it makes a lot more sense to newtype these and provide a more stable interface so that the implementations can be changed without affecting the public API. For instance, being able to switch from using num-bigint to ibig for parsing variable-sized integers could provide a significant speed increase, avoiding unnecessary allocations for small integers. However, changing the Integer alias to IBig would be a breaking change. Reexporting types also makes the docs quite a bit more confusing in my opinion, as the documentation for the types ends up using a completely, totally different type name, for example when I go to view the docs for OctetString, I'm met with the following docs:

A cheaply cloneable and sliceable chunk of contiguous memory.

Bytes is an efficient container for storing and operating on contiguous slices of memory. It is intended for use primarily in networking code, but could have applications elsewhere as well.

Bytes values facilitate zero-copy network programming by allowing multiple Bytes objects to point to the same underlying memory.

Which is a bit jarring when the type signature is named something completely different IMO

@XAMPPRocky
Copy link
Collaborator

XAMPPRocky commented Mar 4, 2022

Thank you for your issue! This is definitely planned, as I'm currently working on implementing PER and constraints, and constraints I want to eventually move the constraints impl to being completely const based, which requires having our own types to be able to add const parameters.

However, changing the Integer alias to IBig would be a breaking change.

I do want to be clear however that this is acceptable. This crate is still very firmly in 0.x phase of development. So preventing breakage is not priority compared to iterating and improving rasn's features and performance.

Which is a bit jarring when the type signature is named something completely different IMO

Yes, seperately I'd like to be able to massively revamp documentation, to provide more both informative and normative information on ASN.1, such that rasn's documentation is the primaary if not only source you need to use ASN.1 successfully.

@XAMPPRocky XAMPPRocky added the kind/enhancement New feature or request label Mar 4, 2022
@XAMPPRocky XAMPPRocky changed the title Consider newtyping types instead of reexporting directly Replace type aliases with newtypes Mar 4, 2022
@XAMPPRocky XAMPPRocky added area/types Related to rasn’s types for ASN.1 help wanted Extra attention is needed labels May 7, 2022
@Nicceboy
Copy link
Contributor

Nicceboy commented Aug 23, 2023

I think I could start working with this [newtype support] after I finish OER codec, if that is okay.

I am still quite new for Rust, so I might need some design help.

But the basic idea would be to use Crate level features and with that to change types?

For the higher type (Integer) and user facing API, we write our own functions, and then with crate features possibly map to different inner types.

@XAMPPRocky
Copy link
Collaborator

I am still quite new for Rust, so I might need some design help.

Right now we're mostly limited by Rust's const capabilities, once we have the ability to use enums it will make the design much more intuitive and straightforward.

You can see a simple version of this with the fixed octet string type.

https://github.com/XAMPPRocky/rasn/blob/a7a00c76d702e530a14d517d830adc899bd31dc4/src/types/strings/octet.rs#L7-L8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/types Related to rasn’s types for ASN.1 help wanted Extra attention is needed kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants