search

ebarnard / rust-plist

A rusty plist parser.
MIT License
71 stars 42 forks source link

Don't clone Value when serializing #62

Closed cmyr closed 3 years ago

cmyr commented 3 years ago

This is a sketch broken out from the discussion in #61.

The only tricky part here is Event, which needs to be owned when deserializing (not technically in the XML case, but practically, since Value is owned) but which we want to have be borrowed when serializing. I've got this working with a Cow; the slightly icky bit is that I need to explicitly specify that the Cow has a 'static lifetime in the deserializing code.

That said, I think it's a reasonable solution?

If this is interesting to you I'd be happy to touch up some docs etc and.

I also haven't actually profiled the difference, although it should be measurable? I'd do that before wanting something like this merged.

ebarnard commented 3 years ago

Looks like a good approach.

I'd be tempted to use type OwnedEvent = Event<'static> for readability but that's not very important.

Can't imagine this will have a huge effect on performance. A slight increase in the size of Event and a few more branches, but that's about it. Saying that, it would be nice if plist had a few benchmarks.

cmyr commented 3 years ago

Alright, I've added the OwnedEvent alias and did a bit of cleanup.

I renamed into_events to to_events, but it might be that just events is a better name? Do you have any opinions?

ebarnard commented 3 years ago

I think the names events() and Events<'a> are the most in keeping with Rust's stdlib naming conventions, but maybe I'm just being picky.

cmyr commented 3 years ago

Nope I think those are better names, I've made that change. This is hopefully ready to go?

ebarnard commented 3 years ago

It is, thanks.

ebarnard commented 3 years ago

Released in 1.2.0