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 OneOf structs readonly #129

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

XanderStoffels
Copy link
Contributor

@XanderStoffels XanderStoffels commented Oct 27, 2022

Solves #120, improving performance when passing a struct to a method using the in keyword.

MS Docs
Blogpost with more insight
Stack overflow discussion

TL;DR

Using in for a parameter lets us pass by reference, but then prevents us from assigning a value to it. However the performance can actually become worse, because it creates a "defensive copy" of the struct, copying the whole thing. A way around this is to use read-only for a struct. When you pass it into an in parameter, the compiler sees that it's read-only and won't create the defensive copy, thereby making it the better alternative for performance.

@XanderStoffels
Copy link
Contributor Author

XanderStoffels commented Oct 27, 2022

On a side note, tests LeftSideFormatsWithCurrentCulture("en-NZ") and RightSideFormatsWithCurrentCulture("en-NZ") seem to be failing for me on the master branch before I introduced changes, so I left those alone. All other tests pass.

Edit: According to appveyor, all tests seem to pass. The tests that fail locally for me are datetime culture related, so Ill keep it at that.

Copy link

@wdolek wdolek left a comment

Choose a reason for hiding this comment

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

If I was in charge, I would approve and release, this is simple yet powerful change.

@mcintyre321 mcintyre321 merged commit 7cd022c into mcintyre321:master Mar 14, 2023
@XanderStoffels XanderStoffels deleted the readonly-structs branch March 14, 2023 13:49
@cremor cremor mentioned this pull request Jul 6, 2023
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.

3 participants