Skip to content

Conversation

Twey
Copy link

@Twey Twey commented Nov 14, 2023

This simplifies the lifetimes used in the Serializer and Deserializer structs. Previous behaviour remains by instantiating R = &mut R_.

Also fixes a few new Clippy lints.

@Twey Twey force-pushed the simplify-lifetimes branch from 4ee3e76 to 3247313 Compare November 21, 2023 15:39
Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

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

@Twey Looks promising but it doesn't build in CI.

@Twey Twey force-pushed the simplify-lifetimes branch 2 times, most recently from 19134e1 to c4d04d2 Compare June 11, 2025 19:49
@Twey
Copy link
Author

Twey commented Jun 11, 2025

@ma2bd fixed CI (I think!) and also made independent from #8.

@Twey Twey force-pushed the simplify-lifetimes branch from c4d04d2 to 8804cc6 Compare June 11, 2025 23:28
Since `&'a mut R` is `Read` (or `Write`) when `R` is
`Read` (resp. `Write`), there is no need to depend on `<R: ?Sized +
Read> &'a mut R` over just plain `<R: Read> R`.  The caller can still
instantiate `R = &'a mut R_` when a reference is desired (and indeed
this is done here for e.g. `MapDeserializer`).
@Twey Twey force-pushed the simplify-lifetimes branch from 8804cc6 to 4c295b1 Compare June 12, 2025 12:27
src/ser.rs Outdated
Comment on lines 4 to 7
// `Error::other` doesn't exist on the MSRV.
#![allow(clippy::io_other_error)]
// And neither does `clippy::io_other_error`.
#![allow(unknown-lints)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work. Should we just bump the MSRV?

Copy link
Author

@Twey Twey Jun 13, 2025

Choose a reason for hiding this comment

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

Oh I think I just misnamed the lint. Fixed in a2260bf.

We could, but in order to get std::io::Error::other we'd need to bump to 1.74, which is a pretty major bump. I'm okay with it if yous are, but it's not a decision I'd take by myself. OTOH it will steadily become harder to maintain lint compatibility between 1.56 and current Rust.

src/ser.rs Outdated
}

impl<'a, W> ser::Serializer for Serializer<'a, W>
impl<'r, W> ser::Serializer for Serializer<&'r mut W>
Copy link
Contributor

@ma2bd ma2bd Jun 12, 2025

Choose a reason for hiding this comment

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

why 'r ? The rest of the file uses 'a consistently.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in dc996e2.

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.

2 participants