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

Add readonly modifier to standard library structs. #904

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Aug 17, 2023

The remaining non-readonly structs are:

  • Nullable<T> (not sure why)
  • ValueTuple<...> (as it's mutable)

Fixes #893.

@Nigel-Ecma and @RexJaeschke I'd particularly like your feedback on the note. Perhaps it shouldn't even be a note, but normative text?

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

This LGTM.

Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

Change “standard” to ”specification” as per @KalleOlaviNiemitalo (I don’t recall this change myself doing the diff for #900 correctness, it showed up a lot, I'm a “Standard” person myself)

Otherwise LGTM and I pre-approve.

The remaining non-readonly structs are:

- `Nullable<T>` (not sure why)
- `ValueTuple<...>` (as it's mutable)

Fixes dotnet#893.
@jskeet
Copy link
Contributor Author

jskeet commented Aug 18, 2023

Changed standard to specification. I'll "dismiss" Nigel's review given the pre-approval.

@jskeet jskeet dismissed Nigel-Ecma’s stale review August 18, 2023 07:51

Pre-approved as I've made the requested change.

@jskeet jskeet merged commit 0ad8948 into dotnet:draft-v7 Aug 18, 2023
@jskeet jskeet deleted the readonly-structs branch August 18, 2023 07:56
@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Aug 18, 2023

  • Nullable<T> (not sure why)

That was explained in dotnet/corefx#24997 (comment): the Equals(object), GetHashCode(), and ToString() methods of Nullable<T> call the corresponding methods of T, and those may mutate the T instance stored within the Nullable<T>. If Nullable<T> were changed to a readonly struct, then its methods would make defensive copies of T and the mutations would no longer affect the Nullable<T> instance. That difference could break some applications.

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.

Readonly structs in standard library
4 participants