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

Reconsider EncodingRef #905

Open
a10y opened this issue Sep 20, 2024 · 0 comments
Open

Reconsider EncodingRef #905

a10y opened this issue Sep 20, 2024 · 0 comments

Comments

@a10y
Copy link
Contributor

a10y commented Sep 20, 2024

Currently we pass around EncodingRef to represent an encoding. It's type-defed to be a &'static dyn Encoding.

Most of our encodings are zero-sized structs, so this is pretty trivial to create. There are also a relatively small number of builtin encodings.

However, with #886 we added an OpaqueEncoding. This one actually is not zero-sized, but takes a u16 to keep track of the (unknown) encoding ID that it wraps. Instances are dynamically created in response to unknown encodings encountered in deserialization, and currently to keep with EncodingRef, we need to Box::leak to get the static reference.

There are a few alternatives

  • Make Context carry around a set of opaque_encodings. Care would need to be taken to make sure this collection is safe for concurrent updates
  • Change EncodingRef to be an Arc<dyn Encoding>. This will create an allocation for the builtin encodings whereas now there are none. However, there are only 25 builtin encodings, not that much to be allocating.
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

No branches or pull requests

1 participant