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

Various improvements to VirtAddr and PhysAddr. #141

Merged
merged 5 commits into from
Apr 10, 2020

Conversation

m-ou-se
Copy link
Contributor

@m-ou-se m-ou-se commented Apr 10, 2020

VirtAddr::new_unchecked didn't behave like the new_unchecked functions in core which just wrap the value without checks or modifications, but instead behaved like PhysAddr::new_truncate. This change renames it to new_truncate, and adds unsafe new_unchecked new_unsafe functions.

Unfortunately, that does mean it's a breaking change. However, I think following this convention for "unchecked" is important to not cause confusion:

  • When looking for something that fixes/truncates the address, I couldn't find any function that did that. (I didn't think that new_unchecked would do that.)
  • When looking for something that lets me unsafely wrap an address, I used new_unchecked, and was surprised by the compiler warning that the unsafe block was unnecessary.

Also fixes and adds some other small things.

@m-ou-se m-ou-se force-pushed the addr branch 3 times, most recently from 061a70a to 21b9500 Compare April 10, 2020 13:32
@phil-opp
Copy link
Member

Thanks a lot! I fully agree with your reasoning. However, I don't think that it's a good idea to use the same new_unchecked name for the new function because it could silently break user's code. For example, if the new_unchecked function was used inside an unsafe block (or an unsafe fn), the behavior silently changes without any compile time error. So I think a different name would be better to ensure that a compile-time error occurs.

@m-ou-se
Copy link
Contributor Author

m-ou-se commented Apr 10, 2020

Sure. Any suggestions? new_unsafe, or something else? Do you want to keep the old new_unchecked as #[deprecated]?

@phil-opp
Copy link
Member

I don't have any good ideas, but new_unsafe seems fine.

Do you want to keep the old new_unchecked as #[deprecated]?

That's a good idea!

@phil-opp phil-opp added this to the 0.10.0 milestone Apr 10, 2020
The old name suggests it's an unsafe function that wraps the given
address without any checks or modifications, like
NonZeroU64::new_unchecked.

The old name is still available, but #[deprecated].
Like NonZeroU64::new_unchecked, they wrap the given value without any
checks or modifications.
@m-ou-se
Copy link
Contributor Author

m-ou-se commented Apr 10, 2020

Updated. Now it's no longer a breaking change.

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.

Thank you!

@phil-opp phil-opp merged commit 4226f67 into rust-osdev:master Apr 10, 2020
@m-ou-se m-ou-se deleted the addr branch April 10, 2020 13:58
@phil-opp
Copy link
Member

Given that this still involves some churn for users, I would like to release it as a new semver-incompatible version, preferable together with some other breaking changes that I want to make. This might take a few days, I hope this is fine with you.

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.

2 participants