Fix incorrect test for 580 and get rid of allocations in hot path #662
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is useless to implement in actual deserializers, because serde internal deserializers (used to implement flatten of tagged enums) does not inherit that values. The right way to change it is to use serde_test::Compact and serde_test::Readable adapters which would transform every deserializer that can come from actual deserializers
MapValueDeseriaizer and SeqItemDeserializer should emit error on DeEvent::Eof instead of calling Visitor::visit_none(). We delegate emitting appropriate error to the Some variant. The content of deserialize_option! macro was inlined and handling of DeEvent::Eof was removed from MapValueDeseriaizer and SeqItemDeserializer. This does not change existing behavior, because Eof in the middle of struct anyway would be captured by `read_to_end()` call at the end of structure deserialization, but this change will report an error in appropriate place, which would important when we implement error position tracking
… be confused with serde trait
Each consumer of MapAccess should consume it fully, so in the end `read_to_end` actually read only one End tag which name already matches the start name, because we use reader that checks that
…ish second attempt to deserialize
If we would need to hook events reading, have a one place for that is preferable
Mingun
added
serde
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #662 +/- ##
==========================================
+ Coverage 64.61% 64.64% +0.03%
==========================================
Files 36 36
Lines 17294 17291 -3
==========================================
+ Hits 11174 11178 +4
+ Misses 6120 6113 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
dralley
approved these changes
Oct 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first commit fixes the mistake I made when fixing #580, that becomes obvious after some refactoring that coming soon for #567.
Deserialization should consume something from deserializer, otherwise an infinity cycle is possible. That is the reason why fixed
SeqItemDeserializer::deserialize_newtype_structtakenSimpleTypeDeserializerinstead ofself. But when I realized that the problem in the test itself, it become obvious that it should takeself, as it does indeserialize_option, for example.Next commits fixes some pinholes and removes allocation on a hot path. It is expected that XML can have many elements and allocating an array just to read one
Endevent after deserializing it is an overkill.