-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Update for recent compiler changes for ref fields #73466
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
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsSee dotnet/roslyn#62886.
|
|
Are all of these changes necessary in order to successfully compile? If yes, this seems like a fairly significant breaking change, no? Or are these changes only necessary because of related places we're already using scoped? |
...m.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs
Outdated
Show resolved
Hide resolved
Yes, these are changes needed to compile successfully. These are typically methods that have a |
| internal override bool CanHaveMetadata => false; | ||
|
|
||
| protected override void Add(in TElement value, ref ReadStack state) | ||
| protected override void Add(in TElement value, scoped ref ReadStack state) |
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.
Why is scoped required here?
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.
Removed.
stephentoub
left a comment
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.
Other than the comments questioning why certain categories of changes are needed, LGTM.
| where TCollection : ConcurrentQueue<TElement> | ||
| { | ||
| protected override void Add(in TElement value, ref ReadStack state) | ||
| protected override void Add(in TElement value, scoped ref ReadStack state) |
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.
Why is scoped required here?
| where TKey : notnull | ||
| { | ||
| protected override void Add(TKey key, in TValue value, JsonSerializerOptions options, ref ReadStack state) | ||
| protected override void Add(TKey key, in TValue value, JsonSerializerOptions options, scoped ref ReadStack state) |
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.
Why is scoped needed here?
| private readonly bool _isDeserializable = typeof(TCollection).IsAssignableFrom(typeof(List<object?>)); | ||
|
|
||
| protected override void Add(in object? value, ref ReadStack state) | ||
| protected override void Add(in object? value, scoped ref ReadStack state) |
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.
Why is scoped needed here?
| private readonly bool _isDeserializable = typeof(TCollection).IsAssignableFrom(typeof(List<TElement>)); | ||
|
|
||
| protected override void Add(in TElement value, ref ReadStack state) | ||
| protected override void Add(in TElement value, scoped ref ReadStack state) |
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.
Why is scoped needed here?
| where TCollection : ConcurrentStack<TElement> | ||
| { | ||
| protected override void Add(in TElement value, ref ReadStack state) | ||
| protected override void Add(in TElement value, scoped ref ReadStack state) |
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.
Why is scoped needed here?
|
I don't see any failures in the arm64 test. Looks like an infrastructure issue. [15:05:31.5263400] 2022-08-12 15:05:31.506 System.Runtime.Tests[16226:178107346] Tests run: 49815 Passed: 49582 Inconclusive: 0 Failed: 0 Ignored: 137 Skipped: 96 |
See dotnet/roslyn#62886.