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

Rename XCr0 and CR4 flags #275

Merged
merged 2 commits into from Aug 2, 2021
Merged

Rename XCr0 and CR4 flags #275

merged 2 commits into from Aug 2, 2021

Conversation

josephlr
Copy link
Contributor

@josephlr josephlr commented Jul 16, 2021

Breaking changes from #272, part of #262

NOTE: this was changed to only deprecate the new constant names, so it is no longer a breaking change.

@ethindp does this include all the breaking changes from your previous PR?

@josephlr josephlr requested a review from phil-opp July 16, 2021 07:53
@josephlr josephlr changed the title Flags breaking Rename XCr0 and CR4 flags Jul 16, 2021
@ethindp
Copy link
Contributor

ethindp commented Jul 16, 2021 via email

Base automatically changed from flags to master July 17, 2021 20:18
@josephlr josephlr changed the base branch from master to next_merge July 19, 2021 07:14
@josephlr josephlr marked this pull request as ready for review July 19, 2021 07:15
@josephlr
Copy link
Contributor Author

The changes here seem good. We will want to wait until #280 is merged, then we will change the branch of this PR back to next, and merge it.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

I wonder if bitflags allows to annotate individuals flags with #[deprecated]? Then we could deprecate the old flags first as part of a normal release and then remove them in the breaking release? This way, we could let the compiler tell the users how they should adjust their code.

I'm not sure if a deprecation period is worth it though, as this only affects two flags that are probably only used by a small number of people currently.

Deprecate YMM

Signed-off-by: Joe Richey <joerichey@google.com>
Deprecate PROTECTION_KEY

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Contributor Author

josephlr commented Aug 2, 2021

I wonder if bitflags allows to annotate individuals flags with #[deprecated]? Then we could deprecate the old flags first as part of a normal release and then remove them in the breaking release? This way, we could let the compiler tell the users how they should adjust their code.

This is a good idea! I changed the PR to keep the old constants, but mark them as deprecated. The rest of the PR is the same (all internal usage is changed to use the new names). I also changed the base to be the master branch, as this can now be part of a "normal" release.

I'm not sure if a deprecation period is worth it though, as this only affects two flags that are probably only used by a small number of people currently.

I think having a deprecation in 0.14.5 and removal in 0.15 is fine. The change isn't too hard, and it would make it easier for the small number of users to upgrade. It's a short window as-is.

@josephlr josephlr changed the base branch from next_merge to master August 2, 2021 10:04
@josephlr josephlr enabled auto-merge August 2, 2021 10:07
@josephlr josephlr merged commit f8a3d70 into master Aug 2, 2021
@josephlr josephlr deleted the flags-breaking branch August 2, 2021 10:10
@phil-opp phil-opp mentioned this pull request Sep 4, 2021
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.

None yet

3 participants