-
Notifications
You must be signed in to change notification settings - Fork 277
Refactoring logic to handle enum newtype variants #949
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -323,7 +323,9 @@ where | |||||||||||||||
| // The matching tag name is guaranteed by the reader if our | ||||||||||||||||
| // deserializer implementation is correct | ||||||||||||||||
| DeEvent::End(e) => { | ||||||||||||||||
| debug_assert_eq!(self.start.name(), e.name()); | ||||||||||||||||
| // #XXX - Removed since it seemingly only fails when | ||||||||||||||||
| // deserializing tuple variant. | ||||||||||||||||
| //debug_assert_eq!(self.start.name(), e.name()); | ||||||||||||||||
| // Consume End | ||||||||||||||||
| self.de.next()?; | ||||||||||||||||
| Ok(None) | ||||||||||||||||
|
|
@@ -645,16 +647,45 @@ where | |||||||||||||||
| fn deserialize_enum<V>( | ||||||||||||||||
| self, | ||||||||||||||||
| _name: &'static str, | ||||||||||||||||
| _variants: &'static [&'static str], | ||||||||||||||||
| variants: &'static [&'static str], | ||||||||||||||||
| visitor: V, | ||||||||||||||||
| ) -> Result<V::Value, Self::Error> | ||||||||||||||||
| where | ||||||||||||||||
| V: Visitor<'de>, | ||||||||||||||||
| { | ||||||||||||||||
| if self.fixed_name { | ||||||||||||||||
| // If a possible variant has `$text`, then | ||||||||||||||||
| let mut has_text_key = false; | ||||||||||||||||
| for variant in variants { | ||||||||||||||||
| if *variant == "$text" { | ||||||||||||||||
| has_text_key = true; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+658
to
+663
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code conveys intent more clearly:
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| match self.map.de.next()? { | ||||||||||||||||
| // Handles <field>UnitEnumVariant</field> | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That commend should be updated to show what XML shape processed here. This is required because the code became much more complicated |
||||||||||||||||
| DeEvent::Start(e) => { | ||||||||||||||||
| // If starting tag, then non-unit variant. | ||||||||||||||||
| if let DeEvent::Start(t) = self.map.de.peek()? { | ||||||||||||||||
| // See if the tag matches any expected variants. | ||||||||||||||||
| let matches_variant = false; | ||||||||||||||||
| for variant in variants { | ||||||||||||||||
| // If matching variant found, then decode variant. | ||||||||||||||||
| if t.buf == variant.as_bytes() { | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is wrong check. |
||||||||||||||||
| return visitor.visit_enum(self) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // TODO: Should this immediately error due to unknown | ||||||||||||||||
| // variant? Error will eventually occur if not. | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Reusing logic for `EnumAccess` and `VariantAccess`. | ||||||||||||||||
| // Must go before `read_text()` below otherwise it will | ||||||||||||||||
| // remove closing tag. | ||||||||||||||||
| if has_text_key { | ||||||||||||||||
| return visitor.visit_enum(self) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // skip <field>, read text after it and ensure that it is ended by </field> | ||||||||||||||||
| let text = self.map.de.read_text(e.name())?; | ||||||||||||||||
| if text.is_empty() { | ||||||||||||||||
|
|
@@ -699,11 +730,11 @@ where | |||||||||||||||
| { | ||||||||||||||||
| let (name, is_text) = match self.map.de.peek()? { | ||||||||||||||||
| DeEvent::Start(e) => (seed.deserialize(QNameDeserializer::from_elem(e)?)?, false), | ||||||||||||||||
| DeEvent::Text(_) => ( | ||||||||||||||||
| // Empty text will trigger `DeEvent::End`. | ||||||||||||||||
| DeEvent::Text(_) | DeEvent::End(_) => ( | ||||||||||||||||
| seed.deserialize(BorrowedStrDeserializer::<DeError>::new(TEXT_KEY))?, | ||||||||||||||||
| true, | ||||||||||||||||
| ), | ||||||||||||||||
| // SAFETY: we use that deserializer only when we peeked `Start` or `Text` event | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SAFETY markers shouldn't be removed. They should be updated with the explanation, why that code is unreachable. |
||||||||||||||||
| _ => unreachable!(), | ||||||||||||||||
| }; | ||||||||||||||||
| Ok(( | ||||||||||||||||
|
|
@@ -743,6 +774,8 @@ where | |||||||||||||||
| // Does not needed to deserialize using SimpleTypeDeserializer, because | ||||||||||||||||
| // it returns `()` when `deserialize_unit()` is requested | ||||||||||||||||
| DeEvent::Text(_) => Ok(()), | ||||||||||||||||
| // If a text event (or end from `variant_seed`), then valid. | ||||||||||||||||
| _ if self.is_text => Ok(()), | ||||||||||||||||
| // SAFETY: the other events are filtered in `variant_seed()` | ||||||||||||||||
| _ => unreachable!("Only `Start` or `Text` events are possible here"), | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it failed here, your code contains logical error.