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

Add length_field_type to LengthDelimitedCodec builder #4508

Merged

Conversation

nvzqz
Copy link
Contributor

@nvzqz nvzqz commented Feb 16, 2022

Like length_field_length except you pass a compile-time checked unsigned integer type instead.

Motivation

Powers of two are the most common values for length_field_length, but passing in a number of bytes for these feels a bit awkward, especially when the docs state "parse a u16 length field".

Solution

Add length_field_type method that takes a generic type.

Implement a sealed (public in private) trait for acceptable types, failing on bad types at compile-time (e.g. i32, u128, String).

Allows passing in the wanted unsigned integer type.
`u128` is analogous to `length_field_length(16)`, which would fail.
@nvzqz
Copy link
Contributor Author

nvzqz commented Feb 18, 2022

r? @hawkw

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this seems good to have to me! it would be nice to get a +1 from someone else as well to make sure there are no API issues that i've overlooked...

/// .length_field_type::<u32>()
/// .new_read(io);
/// # }
/// # pub fn main() {}
Copy link
Member

Choose a reason for hiding this comment

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

my guess is that the fn main()s in these doctests are here because other functions in the module have them? AFAICT, they shouldn't actually be necessary; not a blocker though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I figured the same but wanted to just be consistent with the other doctests.

@nvzqz
Copy link
Contributor Author

nvzqz commented Feb 18, 2022

Thanks Eliza!

r? @carllerche since you touched length_field_length last prior to Eliza.

@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec labels Feb 19, 2022
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

LGTM. Should we actually get Carl to review this?

Comment on lines +800 to +801
/// The default type is [`u32`]. The max type is [`u64`] (or [`usize`] on
/// 64-bit targets).
Copy link
Contributor

Choose a reason for hiding this comment

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

The example uses u128?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compilation failure example uses u128.

@Darksonn Darksonn merged commit 3f508d1 into tokio-rs:master Feb 23, 2022
@nvzqz nvzqz deleted the LengthDelimitedCodec-length_field_type branch February 23, 2022 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants