Skip to content
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

Transparent newtype struct representation #138

Closed
wants to merge 4 commits into from

Conversation

SX91
Copy link
Contributor

@SX91 SX91 commented Jul 20, 2017

#129

Changed encoding of struct A(B) from [B] to B.
This PR is for newtype struct only. Variants and units are harder, so I'd like to discuss it first.

@3Hren
Copy link
Owner

3Hren commented Jul 20, 2017

Thanks! This is a breaking change (however, valid), so I need to test all my dependencies, whether it breaks it and fix them if so.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 87.139% when pulling 19f32ed on SX91:compact-struct into d5cfd5a on 3Hren:master.

@SX91
Copy link
Contributor Author

SX91 commented Jul 21, 2017

I would like to change enum's representation to something like:

[idx] for unit variant
[idx, value] for newtype variant
[idx, [...]] for other

@3Hren what do you think? That would make it compatible with Haskell's data-msgpack library and optimize complex structures layout a lot (reduce more of those nested arrays).

@3Hren
Copy link
Owner

3Hren commented Jul 21, 2017

Again, I'm not strictly agains it. All that matters - is backward compatibility. I think we should consider some kind of swappable policies for serialization. Deserialization must support all these ambiguous cases.

}
other => Err(de::Error::invalid_type(other.unexpected(), &"array")),
}
visitor.visit_newtype_struct(self)
Copy link
Owner

Choose a reason for hiding this comment

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

It is possible to support both old and new deserialization schemes with the cost of some CPU cycles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it would introduce some ambiguity and make it difficult to work with nested types. Handling of something like struct A(Vec<i32>) would require some code to decode it correctly.
Is it really necessary?

@SX91
Copy link
Contributor Author

SX91 commented Sep 1, 2017

@3Hren what needs to be done to get this merged?

@3Hren
Copy link
Owner

3Hren commented Sep 1, 2017

I'll check in 2 days. Serialization is fine, need to check deserialization. Completely burrowed into other projects :(

@3Hren
Copy link
Owner

3Hren commented Sep 12, 2017

At last I have some free time. Checked - no breaks for me! LGTM.

Variants, I think, must behave the same way. Units are doubtful, yeah. Generally speaking for serialization I want to provide options how to serialize ambiguous things, like unit structs. On the other hand deserialization must support all possible variants automatically.

I'll do these myself.

@3Hren
Copy link
Owner

3Hren commented Sep 20, 2017

Added in #146.

@3Hren 3Hren closed this Sep 20, 2017
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.

None yet

3 participants