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

Add stronger alternatives to align_to #105296

Closed
wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 5, 2022

r? @ghost

some code for the discussion

ACP: rust-lang/libs-team#164

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 5, 2022
@rust-log-analyzer

This comment has been minimized.

library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
@Lokathor
Copy link
Contributor

Lokathor commented Dec 5, 2022

One fn seems to change to bigger elements and the other to smaller elements. I'm not clear why it's two separate functions, and also the naming doesn't really suggest which is which.

@scottmcm
Copy link
Member

scottmcm commented Dec 5, 2022

Minor naming thought: Would it be worth leaving space for a future safe-transmute version to have the "ideal" name, and have the unsafe versions be _unchecked or similar?

(Perhaps transmute is enough to communicate the unsafety, though, and we'll use a different word for the safe versions.)

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 21, 2022

One fn seems to change to bigger elements and the other to smaller elements. I'm not clear why it's two separate functions, and also the naming doesn't really suggest which is which.

the transmute* methods are now for all kinds of size changes and document where they won't work (if the alignment gets bigger) and suggest to use the other method instead.

@jswrenn
Copy link
Member

jswrenn commented Dec 21, 2022

Minor naming thought: Would it be worth leaving space for a future safe-transmute version to have the "ideal" name, and have the unsafe versions be _unchecked or similar?

(Perhaps transmute is enough to communicate the unsafety, though, and we'll use a different word for the safe versions.)

FWIW, I'm leaning towards suggesting that the safe transmute APIs use the word "reinterpret" instead of "transmute".

@scottmcm
Copy link
Member

@jswrenn Sounds great -- I've long been a fan of that choice :)

@oli-obk oli-obk marked this pull request as ready for review January 17, 2023 08:57
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 17, 2023

@rustbot label +T-libs-api -T-libs

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 17, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 17, 2023

r? libs-api

#[must_use]
#[unstable(feature = "slice_align_to_ish", issue = "none")]
#[inline]
pub fn aligned_subslice(&self, align: usize) -> &[T] {
Copy link
Member

Choose a reason for hiding this comment

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

Could the align be const? Does one ever need dynamic alignment?

Comment on lines +3736 to +3746
if size_u > size_t {
assert!(
self.len() * size_u % size_t == 0,
"input slice does not fit exactly into a slice of the output element type"
);
} else {
assert!(
self.len() * size_t % size_u == 0,
"input slice does not fit exactly into a slice of the output element type"
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Do the panics get eliminated when the types are exact multiples in size and alignment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I should optimize that.

@anden3 anden3 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2023
@anden3
Copy link
Contributor

anden3 commented Apr 5, 2023

Hello @oli-obk! I just want to ping you as part of the triage procedure to let you know that this PR has received reviews :)

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 6, 2023

The PR is blocked on rust-lang/libs-team#164

@oli-obk oli-obk added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2023
@Dylan-DPC Dylan-DPC added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels May 17, 2023
align % size == 0,
"aligned_subslice only works for alignments that are multiples of the element's size"
);
let offset = self.as_ptr().addr() % align;
Copy link
Member

Choose a reason for hiding this comment

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

If this align is ever a non-constant it'll have to actually emit the division.

Consider taking https://doc.rust-lang.org/std/ptr/struct.Alignment.html instead so you can optimize based on it always being a power of two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.