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
Sprinkle some more readonly around #81297
Conversation
Tagging subscribers to this area: @dotnet/area-meta Issue Detailsnull
|
src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/QCallHandles.cs
Outdated
Show resolved
Hide resolved
All the time I'd like to ask if there's a performance/gc difference between a "read-only" or non-read-only field? Can this be optimized at runtime (e.g. null-checking if readonly field is already set and can never be null after that)? Or is it just syntax to reflect the programmer's intent? |
There can be a perf benefit, in particular for static fields. |
src/libraries/System.Private.CoreLib/src/System/Globalization/OrdinalCasing.Icu.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/OrdinalCasing.Icu.cs
Show resolved
Hide resolved
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.
Changes to types in my ownership area look correct.
Generally scanned the other changes too and they also look sensible, but should likely get secondary eyes from the respective owners.
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.
Scanning through, this all looks straightforward and I presume the compiler would have yelled if anything was amiss.
@stephentoub I'm curious how you went about identifying these sites. Are there more layers of sprinkles to add?
Mostly. The main risk would be making a field containing a mutable struct readonly; the compiler wouldn't complain but it could have bad behavioral impact.
IDE0044. Unfortunately it's currently too noisy for us to enable outright, but I did locally and addressed many of the valid cases. |
@@ -11,7 +11,7 @@ namespace System.Globalization | |||
public sealed class SortVersion : IEquatable<SortVersion?> | |||
{ | |||
private readonly int m_NlsVersion; // Do not rename (binary serialization) | |||
private Guid m_SortId; // Do not rename (binary serialization) | |||
private readonly Guid m_SortId; // Do not rename (binary serialization) |
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.
Does not this break serialization support?
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.
I don't believe it does. @GrabYourPitchforks?
No description provided.