downgrade ptr.is_aligned_to crate-private #121920
Conversation
rustbot
added
O-SGX
|
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
rustbot
added
O-hermit
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -37,7 +37,7 @@ fn main() { | |||
| let size = 31; | |||
| assert_eq!(libc::posix_memalign(&mut ptr, align, size), 0); | |||
| assert!(!ptr.is_null()); | |||
| assert!(ptr.is_aligned_to(align)); | |||
There was a problem hiding this comment.
This does seem like the function can be quite nice to have, doesn't it? ptr.is_aligned_to(align) is a lot easier to read than the modulo check.
And if it's used heavily inside the standard library (and hence can't be removed entirely, only be made private), that is also a data point in favor of it actually being useful outside of the standard library. So I don't quite see why it should get removed.
There was a problem hiding this comment.
It seems to only be used for asserts. Maybe we should have a new assert macro?
There was a problem hiding this comment.
In general, an application developer probably wants .is_aligned_to::<N>. It is rare that you truly want a dynamic alignment check.
There was a problem hiding this comment.
so it's only used heavily in the stdlib at this point because:
- people not being aware how powerful
ptr.cast::<T>().some_check()is as an idiom - assert_unsafe_precondition was added in a ton of places calling is_aligned_and_not_null which calls this
The former I rewrote, the latter is a bit ??? to me. The functions/macros are clearly written in a very specific style that avoids generics, and I can't tell if this is Important For Something or someone being idiosyncratic for no good reason.
There was a problem hiding this comment.
Sorry just to clarify: as used is_aligned_and_not_null could just be changed to be generic and call .is_aligned(). But I'm not sure if that would break some magic.
There was a problem hiding this comment.
also Extremely funny saethlin is the one who suggested removing it X3
There was a problem hiding this comment.
🤨 I don't and didn't mean that we should never stabilize such a function. But at the current time, I think we have evidence that this doesn't meet the bar for what goes in the standard library.
In particular, I find the API of this function quite vexing. The usize argument raises annoying questions because not every usize is a valid alignment. The design needs to commit to one of
- Don't try to detect alignments that aren't a power of 2. I dislike this option because we shouldn't let trivial-to-detect bugs go unnoticed. A debug assertion would get compiled out of the distributed standard library and therefore not really exist for most users so I'm not really in favor of that.
- Panic when passed an alignment that's not a power of 2. I dislike this option because generates a substantial amount of IR and I expect that if this function is used by performance nerds like me the minor inefficiency will invite people to roll their own and advocate against using the standard library function. At which point, why are we stabilizing something people will replace in a third-party library.
- Make the alignment a
constparam like Jubilee says above. This would rule out 2 of the 3 users of a function calledis_aligned_tothat I linked in the tracking issue. So I'm not particularly convinced that there are many users of aconst ALIGNversion, considering that the 1 user who could use such an API can also useis_aligned::<T>(). - Change the argument type to an
Alignment. This seems to me like the right way to design a Rust API, because it rules out invalid usage with the type system. This option also increases the difficulty of stabilizing something, but that seems fine when the other options are stabilizing something with a high chance of regret.
There was a problem hiding this comment.
To be clear I think it's perfectly fine to stabilize is_aligned (which checks the pointee type, right?) and leave is_aligned_to unstable, if there is consensus that is_aligned is sufficiently useful and will remain useful even if is_aligned_to eventually becomes stable. That would require moving them to different feature gates.
I am just surprised about the proposal of making them private. That's a pretty rare thing to be done with a unstable std API I think. Usually it either gets removed or stabilized.
There was a problem hiding this comment.
Ah I guess you're already doing that in #121948. :)
There was a problem hiding this comment.
I'll add my 2¢: I think is_aligned_to is useful (even if much less than plain is_aligned), as suggested by the uses in std and other potential uses in the ecosystem. But I also agree that accepting usize is not nice.
IMO we should change it to accepting Alignment and stabilize is_aligned separately.
Per rust-lang#96284 (comment) this API is a mess and is regarded as insufficiently motivated. Removing it from the public API is potentially the only blocker on stabilizing its pleasant brother .is_aligned(). I initially just deleted it completely but it's since become Heavily Used for the purposes of assert_unsafe_precondition, and there's clearly Some Stuff going on with that API that I have no interest in poking.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
(PR currently failing because: there's a codegen test for this function, which, is still desirable in a sense. Leaving the PR as-is for the sake of discussing whether "insufficiently motivated" is still true.) |
This is an alternative to rust-lang#121920
This is an alternative to rust-lang#121920
m-ou-se
added
T-libs-api
|
☔ The latest upstream changes (presumably #122045) made this pull request unmergeable. Please resolve the merge conflicts. |
stabilize ptr.is_aligned, move ptr.is_aligned_to to a new feature gate This is an alternative to rust-lang#121920
|
Libs-api discussed this and is happy to remove this unstable API by making is crate-private. @rfcbot fcp merge |
|
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
Amanieu
removed
the
I-libs-api-nominated
rfcbot
added
the
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
removed
the
proposed-final-comment-period
rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
|
uhhh this pr is a strict alternative to #121948, which is also accepted? (i rebased that one, it's... strictly more pleasant, i think?) |
This is an alternative to rust-lang#121920
stabilize ptr.is_aligned, move ptr.is_aligned_to to a new feature gate This is an alternative to rust-lang#121920
|
Agreed, we should land #121948, not this. @rust-lang/libs-api you FCP-approved two PRs that contradict each other about how |
RalfJung
added
the
I-libs-api-nominated
|
I believe the point of the FCP on #121948 was to accept stabilization of |
|
The author of this PR (and other people, like myself) no longer see a need to downgrade |
|
Taking into account #96284 (comment), those of us in today's libs-api meeting didn't see If, in short order, we can find someone who volunteers that they plan to write an ACP justifying |
Amanieu
removed
the
I-libs-api-nominated
Per #96284 (comment) this API is a mess and is regarded as insufficiently motivated. Removing it from the public API is potentially the only blocker on stabilizing its pleasant brother .is_aligned().
I initially just deleted it completely but it's since become Heavily Used for the purposes of assert_unsafe_precondition, and there's clearly Some Stuff going on with that API that I have no interest in poking.