-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Mark applicable structs as readonly #14789
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,7 +53,7 @@ namespace System | |
| [StructLayout(LayoutKind.Auto)] | ||
| [Serializable] | ||
| [System.Runtime.CompilerServices.TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")] | ||
| public partial struct DateTime : IComparable, IFormattable, IConvertible, IComparable<DateTime>, IEquatable<DateTime>, ISerializable | ||
| public readonly partial struct DateTime : IComparable, IFormattable, IConvertible, IComparable<DateTime>, IEquatable<DateTime>, ISerializable | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that you were not able to mark DateTimeOffset because of its internal implementation detail around binary serialization. It is pretty unfortunate. I am wondering whether the DateTimeOffset case would warrant using the Unsafe helper to cast away the readonly-liness, so that DateTimeOffset can be marked as well.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. I started looking at fixing it, but decided to keep this PR just focused on the easy cases where implementation changes weren't necessary. There are probably also additional structs that could be made readonly if their fields were appropriately annotated; I only looked at / fixed a few in that category (e.g. DateTime, Nullable), but didn't do a full sweep looking for others; that can be done subsequently, either piecemeal or with an analyzer.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DateTimeOffset #19552 |
||
| { | ||
| // Number of 100ns ticks per time unit | ||
| private const long TicksPerMillisecond = 10000; | ||
|
|
@@ -135,7 +135,7 @@ public partial struct DateTime : IComparable, IFormattable, IConvertible, ICompa | |
| // savings time hour and it is in daylight savings time. This allows distinction of these | ||
| // otherwise ambiguous local times and prevents data loss when round tripping from Local to | ||
| // UTC time. | ||
| private UInt64 _dateData; | ||
| private readonly UInt64 _dateData; | ||
|
|
||
| // Constructs a DateTime from a tick count. The ticks | ||
| // argument specifies the date as the number of 100-nanosecond intervals | ||
|
|
||
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'm working on the APICompat checks work but I'm not sure if this a compatible change. @jkotas @stephentoub is this an API compatible change?
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.
Marking struct as readonly should be compatible change. The other direction (removing readonly on a struct) is not a compatible change.
cc @VSadov
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.
Yes this is a compatible change.