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.

Sign up for GitHub

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

Jump to bottom

Make Thread.VolatileRead and Thread.VolatileWrite obsolete #99382

Merged
merged 5 commits into from
Mar 8, 2024
Merged

Make Thread.VolatileRead and Thread.VolatileWrite obsolete #99382

merged 5 commits into from
Mar 8, 2024

Conversation

CollinAlpert
Copy link
Contributor

This PR obsoletes Thread.VolatileRead as well as Thread.VolatileWrite in favor of Volatile.Read and Volatile.Write respectively, as per #27997.

I am implementing the fixer for the diagnostic ID in dotnet/roslyn-analyzers#7043. The main reason I want to get this PR merged before completing the fixer, is to confirm SYSLIB0054 as the diagnostic ID.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 6, 2024
@CollinAlpert CollinAlpert changed the title Issue 27997 Make Thread.VolatileRead and Thread.VolatileWrite obsolete Mar 6, 2024
@vcsjones vcsjones added area-System.Threading and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 6, 2024
@ghost
Copy link

ghost commented Mar 6, 2024

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR obsoletes Thread.VolatileRead as well as Thread.VolatileWrite in favor of Volatile.Read and Volatile.Write respectively, as per #27997.

I am implementing the fixer for the diagnostic ID in dotnet/roslyn-analyzers#7043. The main reason I want to get this PR merged before completing the fixer, is to confirm SYSLIB0054 as the diagnostic ID.

Author: CollinAlpert
Assignees: -
Labels:

area-System.Threading
Milestone: -

This was referenced Mar 7, 2024
Closed
Closed
stephentoub
stephentoub reviewed Mar 7, 2024
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than the feedback about EditorBrowsable, LGTM. Thanks.

buyaa-n
buyaa-n approved these changes Mar 8, 2024
View reviewed changes
Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM Thank you!

@buyaa-n buyaa-n merged commit 6ebc8bd into dotnet:main Mar 8, 2024
177 of 179 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2024
@gewarren
Copy link
Contributor

gewarren commented May 2, 2024

Should this have the needs breaking change doc label?

@vcsjones vcsjones added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label May 2, 2024
@dotnet dotnet unlocked this conversation May 2, 2024
@stephentoub
Copy link
Member

@jeffhandley, @ericstj, do we treat every [Obsolete] as a breaking change to be documented?

@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label May 2, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@ericstj
Copy link
Member

ericstj commented May 2, 2024

Yes, since it will be source breaking (new warnings - for some as error). Of course this one is a low impact one since it just requires a rename and a fixer is planned. If we don't want the breaking change then don't make the APIs obsolete.

@gewarren
Copy link
Contributor

gewarren commented May 9, 2024

The breaking change doc is taken care of with dotnet/docs#40834.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2024
@CollinAlpert CollinAlpert deleted the issue-27997 branch June 17, 2024 11:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vcsjones vcsjones vcsjones left review comments

@stephentoub stephentoub stephentoub left review comments

@buyaa-n buyaa-n buyaa-n approved these changes

@jeffhandley jeffhandley Awaiting requested review from jeffhandley jeffhandley is a code owner

Assignees
No one assigned
Labels
area-System.Threading breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@CollinAlpert @gewarren @stephentoub @ericstj @vcsjones @buyaa-n