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

Tracking Issue for ptr::Alignment type #102070

Open
3 of 6 tasks
scottmcm opened this issue Sep 20, 2022 · 4 comments
Open
3 of 6 tasks

Tracking Issue for ptr::Alignment type #102070

scottmcm opened this issue Sep 20, 2022 · 4 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

scottmcm commented Sep 20, 2022

Feature gate: #![feature(ptr_alignment_type)]

This is a tracking issue for the ptr::Alignment type, and related methods, to represent values that are valid alignments in the rust abstract machine.

Public API

// core::ptr

#[derive(Copy, Clone, Eq, PartialEq)]
pub struct Alignment();

impl Debug for Alignment;
impl Ord for Alignment;
impl PartialOrd for Alignment;
impl Hash for Alignment;

impl Alignment {
    pub const MIN: Self;
    pub const fn of<T>() -> Self;
    pub const fn new(align: usize) -> Option<Self>;
    pub const unsafe fn new_unchecked(align: usize) -> Self;
    pub const fn as_usize(self) -> usize;
    pub const fn as_nonzero(self) -> NonZeroUsize;
    pub const fn log2(self) -> u32;
}

impl TryFrom<NonZeroUsize> for Alignment;
impl TryFrom<usize> for Alignment;
impl From<Alignment> for NonZeroUsize;
impl From<Alignment> for usize;

Steps / History

Unresolved Questions

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Sep 20, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 9, 2022
Add `ptr::Alignment` type

Essentially no new code here, just exposing the previously-`pub(crate)` `ValidAlign` type under the name from the ACP.

ACP: rust-lang/libs-team#108
Tracking Issue: rust-lang#102070

r? `@ghost`
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 9, 2022
Add `ptr::Alignment` type

Essentially no new code here, just exposing the previously-`pub(crate)` `ValidAlign` type under the name from the ACP.

ACP: rust-lang/libs-team#108
Tracking Issue: rust-lang#102070

r? ``@ghost``
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 10, 2022
…thomcc

From<Alignment> for usize & NonZeroUsize

Since you mentioned these two in rust-lang#102072 (comment),
r? `@thomcc`

Tracking Issue: rust-lang#102070
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 10, 2022
…thomcc

From<Alignment> for usize & NonZeroUsize

Since you mentioned these two in rust-lang#102072 (comment),
r? ``@thomcc``

Tracking Issue: rust-lang#102070
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 12, 2022
Remove the old `ValidAlign` name

Since it looks like there won't be any reverts needed in `Layout` for rust-lang#101899 (comment), finish off this change that I'd left out of rust-lang#102072.

r? `@thomcc`
cc tracking issue rust-lang#102070
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 12, 2022
Remove the old `ValidAlign` name

Since it looks like there won't be any reverts needed in `Layout` for rust-lang#101899 (comment), finish off this change that I'd left out of rust-lang#102072.

r? ``@thomcc``
cc tracking issue rust-lang#102070
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
Add `ptr::Alignment` type

Essentially no new code here, just exposing the previously-`pub(crate)` `ValidAlign` type under the name from the ACP.

ACP: rust-lang/libs-team#108
Tracking Issue: rust-lang/rust#102070

r? ``@ghost``
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
From<Alignment> for usize & NonZeroUsize

Since you mentioned these two in rust-lang/rust#102072 (comment),
r? ``@thomcc``

Tracking Issue: rust-lang/rust#102070
@kupiakos
Copy link
Contributor

I've been able to take advantage of an alignment newtype for performance in an embedded arena library and would find this useful to be stabilized. Are there any blockers for an FCP?

@scottmcm scottmcm added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jul 22, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Jul 25, 2023

We discussed this in the libs-api meeting just now. We think this type should be used in std APIs (e.g. methods of Layout) before stabilizing the type. Right now, this type doesn't seem to be used anywhere (publicly) in std, so we'd like to see a plan for usage of this type in std.

We also briefly talked about the error type in the TryFrom impls. It's not immediately clear if TryFromIntError is the right type. Perhaps it needs a new error type.

@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jul 25, 2023
@clarfonthey
Copy link
Contributor

clarfonthey commented Aug 21, 2023

As I'm trying to make a stable version of this API as a crate for my own use, I have a few questions about the current implementation:

  1. Are alignments above isize::MAX actually allowed, given how most allocations are limited to isize::MAX instead of usize::MAX? This would complicate some of the code a bit (instead of is_power_of_two, you also need to check < isize::MAX), but seems like it would be desired if the goal is to represent actual alignments, and not just powers of two.
  2. If the above is done, there should also be methods to convert to isize infallibly, and NonZeroIsize.
  3. This type should implement Display, Binary, Octal, LowerHex, and UpperHex.
  4. When I actually went to use this type, one case I felt was useful was a DoubledLen type, which includes both the powers of two and zero. The main reason for this was that for a particular hash table implementation, I wanted to assert with types that the length was a power of two, so that you can always mask out bits of an index instead of doing a modulus. Option<Alignment> does work for this, but I decided to go all-out here.
  5. Another thing worth considering is methods for actually doing such masking: when actually checking if an address is aligned, you'll want to create a "mask" for that alignment, which effectively boils down to !(align - 1). Having a method to create this mask as a usize might be nice to have, and it also would be useful for the DoubledLen type I described.

@scottmcm
Copy link
Member Author

  1. Yes, because you can make isize::MAX+1-aligned ZST at the address ptr::invalid(isize::MIN as usize). That's probably not all that useful, but it's legal in the rust AM, as far as I know.
  2. Agreed, having good ways to use this for things like masking (or aligning up/down) are important. I haven't gotten to making specifics for that; I'd encourage you to make some if you have something in mind.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 14, 2023
impl more traits for ptr::Alignment, add mask method

Changes:

* Adds `rustc_const_unstable` attributes where missing
* Makes `log2` method const
* Adds `mask` method
* Implements `Default`, which is equivalent to `Alignment::MIN`

No longer included in PR:

* Removes indirection of `AlignmentEnum` type alias (this was intentional)
* Implements `Display`, `Binary`, `Octal`, `LowerHex`, and `UpperHex` (should go through libs-api instead)
* Controversially implements `LowerExp` and `UpperExp` using `p` instead of `e` to indicate a power of 2 (also should go through libs-api)

Tracking issue for `ptr::Alignment`: rust-lang#102070
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 18, 2023
impl more traits for ptr::Alignment, add mask method

Changes:

* Adds `rustc_const_unstable` attributes where missing
* Makes `log2` method const
* Adds `mask` method
* Implements `Default`, which is equivalent to `Alignment::MIN`

No longer included in PR:

* Removes indirection of `AlignmentEnum` type alias (this was intentional)
* Implements `Display`, `Binary`, `Octal`, `LowerHex`, and `UpperHex` (should go through libs-api instead)
* Controversially implements `LowerExp` and `UpperExp` using `p` instead of `e` to indicate a power of 2 (also should go through libs-api)

Tracking issue for `ptr::Alignment`: rust-lang#102070
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants