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

Allow using Box<[u8]>/Box<str> for bytes/string fields #878

Closed
wants to merge 14 commits into from

Conversation

poliorcetics
Copy link

This reduces the size of objects by one byte each time they're used, which can help quite a lot in some cases.

This is fully optional and the defaults have not changed.

@poliorcetics
Copy link
Author

poliorcetics commented Jun 30, 2023

I saw #429 but it's not what I wanted since it asks for two levels of indirection, Arc<Vec<u8>> for example.

I can probably easily add Arc<str> and Arc<[u8]> if necessary though

@poliorcetics poliorcetics force-pushed the box-the-world branch 3 times, most recently from 1525688 to d8239f2 Compare July 1, 2023 15:39
@poliorcetics
Copy link
Author

I tried to implement this in an external lib but it's inefficient if done naively (conversions to String/Vec<u8>) everywhere and since prost doesn't expose its helpers to build Messages, doing it efficiently means copying a lot of code that is intended for internal use only and is quite brittle when used in another crate.

@michaelvanstraten
Copy link

michaelvanstraten commented Aug 16, 2023

Please merge this, would be extremely helpful.

@poliorcetics
Copy link
Author

Rebased to fix conflicts, no actual changes made

@caspermeijn
Copy link
Collaborator

It seems like #979 is interesting for this PR. It would simplify the encoding needed for Box<str> support, am I right?

@poliorcetics
Copy link
Author

I'm not working with prost anymore and this PR is way out of date, someone interested can retake it if necessary :)

@poliorcetics poliorcetics deleted the box-the-world branch June 3, 2024 12:25
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