Skip to content

Conversation

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Feb 11, 2021

Allows us to avoid extra allocations from querystring parsing.

I don't think this is breaking? But maybe I don't understand the trait inheritance here correctly.

Will also necessitate similar changes in Tide & Surf.

@yoshuawuyts
Copy link
Member

I remember trying this in the early days of surf, and not being able to allocate created all sorts of gnarly lifetime issues. I'm not against this change, but I'd like to see this paired with some extra tests to ensure it in fact does cover all scenarios we want to cover. The risk is that we make this change, and people now need to work around our serde methods because they now only cover a subset of uses they did before.

@Fishrock123
Copy link
Member Author

Fishrock123 commented Feb 23, 2021

I don't follow. As far as I am aware DeserializeOwned is more restrictive than Deserialize<'de>...?

There is an existing owned test case with deserializing into a hashmap of Strings. This adds a borrowing test case. Owned deserializes still work just fine.

@Fishrock123
Copy link
Member Author

Allows us to avoid extra allocations from querystring parsing.
@Fishrock123 Fishrock123 force-pushed the request-query-deserialize-borrowed branch from feeab31 to f5d310a Compare February 25, 2021 18:21
@Fishrock123
Copy link
Member Author

Fishrock123 commented Feb 27, 2021

From the aforelinked docs, particularly that last line (emphasis mine):

The Deserialize<'de> lifetime

This lifetime records the constraints on how long data borrowed by this type must be valid.

Every lifetime of data borrowed by this type must be a bound on the 'de lifetime of its Deserialize impl. If this type borrows data with lifetime 'a, then 'de must be constrained to outlive 'a.

If this type does not borrow any data from the Deserializer, there are simply no bounds on the 'de lifetime. Such types automatically implement the DeserializeOwned trait.

@Fishrock123
Copy link
Member Author

All signs point to this being fine. Merging.

@Fishrock123 Fishrock123 merged commit a906bad into http-rs:main Apr 2, 2021
@Fishrock123 Fishrock123 deleted the request-query-deserialize-borrowed branch April 21, 2021 17: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.

2 participants