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

Make pointer::byte_offset_from more generic #103489

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

WaffleLapkin
Copy link
Member

As suggested by #96283 (comment) (cc @scottmcm), make pointer::byte_offset_from work on pointers of different types. byte_offset_from really doesn't care about pointer types, so this is totally fine and, for example, allows patterns like this:

ptr::addr_of!(x.b).byte_offset_from(ptr::addr_of!(x))

The only possible downside is that this removes the T == U hint to inference, but I don't think this matter much. I don't think there are a lot of cases where you'd want to use byte_offset_from with a pointer of unbounded type (and in such cases you can just specify the type).

@rustbot label +T-libs-api

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 24, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 24, 2022

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

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 24, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2022
@Aaron1011
Copy link
Member

I'm concerned that this makes the API a little easier to misuse. Both pointers need to be in the same allocation with the same provenance, which seems "less likely" if the types are different. I could imagine someone accidentally passing in the wrong pointer, and having their mistake caught due to a type error.

If it's rare in practice to call this with two differently-typed pointers, than I think it would be better tor require an explicit cast on the caller side.

@scottmcm
Copy link
Member

The problem I have here is that if the types are the same, why wouldn't the caller be using offset_from? It's not obvious to me that telling someone to cast their *const i16 to a *const u64 in order to call this is a net win, since potentially-misaligned pointers like that are a slight code odor. (Maybe not even a smell, but still an indication that an extra look is encouraged.) And if it ends up being .byte_offset_from(p.cast()) -- or worse, .byte_offset_from(p as _) -- in most of the calls, that's not necessarily a win. After all, this is just a convenience method to avoid a .cast::<u8>(), so if one needs to cast one of the pointers, maybe just cast both of them.

But I guess it's hard to make a good case either way since there appear to be literally zero uses of byte_offset_from in the rust repo -- not even doctest examples.

So maybe this really needs a code audit to find places that could use this instead of whatever it's currently doing. That accidentally ended up really helpful in #95837, for example, showing that actually nothing wanted offset_from, and thus encouraging a really short name for the returns-usize-instead-of-isize version.

@scottmcm
Copy link
Member

scottmcm commented Nov 9, 2022

@rustbot label +I-libs-api-nominated +S-waiting-on-team

Hi libs-api! I brought this up, so obviously I think it's a good idea, but given objections raised above I'd like to get extra thoughts on it. Would you rather this stay homogeneous, or should it accept mixed types of pointers?

@rustbot rustbot added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 9, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Nov 15, 2022

We discussed this very briefly in the libs-api meeting. This change looks fine to us. :)

@m-ou-se m-ou-se removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Nov 15, 2022
@scottmcm
Copy link
Member

With the ok for nightly from libs-api,
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 15, 2022

📌 Commit 6279d09 has been approved by scottmcm

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2022
… r=scottmcm

Make `pointer::byte_offset_from` more generic

As suggested by rust-lang#96283 (comment) (cc `@scottmcm),` make `pointer::byte_offset_from` work on pointers of different types. `byte_offset_from` really doesn't care about pointer types, so this is totally fine and, for example, allows patterns like this:
```rust
ptr::addr_of!(x.b).byte_offset_from(ptr::addr_of!(x))
```

The only possible downside is that this removes the `T` == `U` hint to inference, but I don't think this matter much. I don't think there are a lot of cases where you'd want to use `byte_offset_from` with a pointer of unbounded type (and in such cases you can just specify the type).

`@rustbot` label +T-libs-api
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Nov 16, 2022
… r=scottmcm

Make `pointer::byte_offset_from` more generic

As suggested by rust-lang#96283 (comment) (cc ``@scottmcm),`` make `pointer::byte_offset_from` work on pointers of different types. `byte_offset_from` really doesn't care about pointer types, so this is totally fine and, for example, allows patterns like this:
```rust
ptr::addr_of!(x.b).byte_offset_from(ptr::addr_of!(x))
```

The only possible downside is that this removes the `T` == `U` hint to inference, but I don't think this matter much. I don't think there are a lot of cases where you'd want to use `byte_offset_from` with a pointer of unbounded type (and in such cases you can just specify the type).

``@rustbot`` label +T-libs-api
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2022
… r=scottmcm

Make `pointer::byte_offset_from` more generic

As suggested by rust-lang#96283 (comment) (cc ```@scottmcm),``` make `pointer::byte_offset_from` work on pointers of different types. `byte_offset_from` really doesn't care about pointer types, so this is totally fine and, for example, allows patterns like this:
```rust
ptr::addr_of!(x.b).byte_offset_from(ptr::addr_of!(x))
```

The only possible downside is that this removes the `T` == `U` hint to inference, but I don't think this matter much. I don't think there are a lot of cases where you'd want to use `byte_offset_from` with a pointer of unbounded type (and in such cases you can just specify the type).

```@rustbot``` label +T-libs-api
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 16, 2022
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#103484 (Add `rust` to `let_underscore_lock` example)
 - rust-lang#103489 (Make `pointer::byte_offset_from` more generic)
 - rust-lang#104193 (Shift no characters when using raw string literals)
 - rust-lang#104348 (Respect visibility & stability of inherent associated types)
 - rust-lang#104401 (avoid memory leak in mpsc test)
 - rust-lang#104419 (Fix test/ui/issues/issue-30490.rs)
 - rust-lang#104424 (rustdoc: remove no-op CSS `.popover { font-size: 1rem }`)
 - rust-lang#104425 (rustdoc: remove no-op CSS `.main-header { justify-content }`)
 - rust-lang#104450 (Fuchsia test suite script fix)
 - rust-lang#104471 (Update PROBLEMATIC_CONSTS in style.rs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 91963cc into rust-lang:master Nov 16, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 16, 2022
@WaffleLapkin WaffleLapkin deleted the byte_offset_from_you branch February 27, 2023 11:37
@scottmcm scottmcm mentioned this pull request Aug 2, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. 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.

8 participants