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

Make structs readonly #1130

Merged
merged 2 commits into from May 20, 2018
Merged

Make structs readonly #1130

merged 2 commits into from May 20, 2018

Conversation

@jskeet
Copy link
Member

@jskeet jskeet commented May 19, 2018

This is the first pass at #1033.

Once #1129 is merged, this is a single commit.

IXmlSerializable is fundamentally broken for readonly structs, but we can cast away the readonly-ness using the System.Runtime.CompilerServices.Unsafe package. It feels like the benefits of this are greater than the downsides.

@malcolmr
Copy link
Contributor

@malcolmr malcolmr commented May 19, 2018

LGTM. Are there any threading or 'undefined behaviour'-style issues with the use of Unsafe.AsRef()?

@jskeet jskeet force-pushed the jskeet:readonly-attempt branch from d903b74 to 8f4e7c5 May 20, 2018
@jskeet
Copy link
Member Author

@jskeet jskeet commented May 20, 2018

Basically we're breaking the readonly contract, with all the fun that could entail. If this is used by anyone outside regular XML serialization, I'd expect problems. Within XML serialization, it should be fine - this it all called on new objects.

I'm going to wait until we've got the first benchmarks working for v3 before merging.

@jskeet jskeet merged commit 0ffe6da into nodatime:master May 20, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jskeet jskeet deleted the jskeet:readonly-attempt branch May 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants