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 sub_ptr (feature ptr_sub_ptr) #95892

Open
1 of 4 tasks
scottmcm opened this issue Apr 10, 2022 · 10 comments
Open
1 of 4 tasks

Tracking Issue for sub_ptr (feature ptr_sub_ptr) #95892

scottmcm opened this issue Apr 10, 2022 · 10 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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 Apr 10, 2022

Feature gate: #![feature(ptr_sub_ptr)] & #![feature(const_ptr_sub_ptr)]

This is a tracking issue for the <*const _>::sub_ptr & <*mut _>::sub_ptr methods.

This is the produces-usize version of offset_from, the same way that add and sub are the takes-usize versions of offset.

It turns out that people almost always actually know which pointer is greater than which when doing this operation, and would rather a usize instead of an isize -- every use of offset_from in the library was followed with as usize in practice. So like how .add(d) greatly improved code compared to needing .offset(d as isize), being able to use ptr.sub_ptr(origin) instead of ptr.offset_from(origin) as usize is also a major improvement. And Miri can check the unsafety better, too, since if you get the order wrong it'll detect that, unlike happens with the as usize approach.

This also tracks the constness of operations, though with #92980 stabilizing offset_from being const , this being const is likely uncontroversial.

Public API

impl<T> *const T {
    pub const unsafe fn sub_ptr(self, origin: *const T) -> usize;
}

impl<T> *mut T {
    pub const unsafe fn sub_ptr(self, origin: *const T) -> usize;
}

Steps / History

Unresolved Questions

@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: A tracking issue for an RFC or an unstable feature. labels Apr 10, 2022
@Stargateur
Copy link
Contributor

Stargateur commented Mar 22, 2023

I will always prefer this to the use of isize. The name is very generic. Thus I don't have better idea. Maybe usize_from or distance_from. I don't like the order of parameter but guess it's too late for pointer.

@Stargateur
Copy link
Contributor

Stargateur commented Mar 22, 2023

I also wonder if we could just say this doesn't panic but just return the absolute since we assert the greater than equal why not just make it return absolute ? this would cost nothing more and would remove the burden for dev to think about order, and we could just call this distance().

@scottmcm
Copy link
Member Author

scottmcm commented Mar 22, 2023

I would be strongly against this returning the absolute difference between the pointers. Part of the value of this API is that it's more efficient than offset_from -- that's why slice iterators were open-coding this before the function existed -- and if it needed to do tests for ordering it would no longer be that.

Adding a separate absolute difference API for pointers might make sense, like the integers have, but it wouldn't be a serviceable replacement for the current sub_ptr. (#106393 wouldn't use an absolute difference version, for example.)

(Oh, and as prior art, std::distance in C++ isn't an absolute distance either, so calling it distance might mislead people.)

@Stargateur
Copy link
Contributor

Stargateur commented Mar 23, 2023

I proposed this cause you write in the PR:

// SAFETY: The comparison has no side-effects, and the intrinsic
// does this check internally in the CTFE implementation.
unsafe { assert_unsafe_precondition!(self >= origin) };

so I expect it to be "free", I'm all in favor of remove all assert since the method is unsafe anyway, this kind of operation should be as fast as possible. I just see this line and wonder, well if it's free why not ?

(Oh, and as prior art, std::distance in C++ isn't an absolute distance either, so calling it distance might mislead people.)

I don't think C++ std using distance name wrongly is a good point, have you ever see "this object is -1cm" ? a distance is always positive, that why we return usize. Distance is also mention many time in #95837.

@scottmcm
Copy link
Member Author

cause you write in the PR:

assert_unsafe_precondition! is a magic compiler thing that only does something in CTFE. At codegen time, sub_ptr doesn't do any checks, just an "I promise not to overflow" subtraction and a "I promise it's an exact multiple" division: https://rust.godbolt.org/z/fE79dv6WT.

@deltragon
Copy link
Contributor

Since offset_from has a parallel byte_offset_from method (tracked in #96283), should this method have an equivalent byte_sub_ptr method?
I am aware that this was added specifically for slice iterators, and there it doesn't seem like there has been a usecase for a bytewise method so far - but looking at the docs it seemed somewhat surprising that this is missing.

@RalfJung
Copy link
Member

Yeah, if this stabilizes after the other byte methods we probably want byte_sub_ptr as well.

@LoganDark
Copy link

I've read some of the bikeshedding in #95837, and I'd like to second @Gankra's suggestion of "offset_to" - it's not perfect, but when I was looking for this method, I couldn't find it in the autocomplete and had to search for the -> usize return type instead. Having offset in the name would have made it easier to locate, as that's one of the first things I looked for.

Are there any blockers for stabilization besides the name? I'm not familiar with the stabilization process, but I'd be willing to open a stabilization PR for this if that would help.

@RalfJung
Copy link
Member

So offset_from returns isize but offset_to returns usize? That makes little sense to me.

Once this is stable we can put a note in the offset_from docs pointing to sub_ptr.

@LoganDark
Copy link

LoganDark commented Nov 12, 2023

So offset_from returns isize but offset_to returns usize? That makes little sense to me.

In isolation, it doesn't make much sense to me either - given the chance to go back in time, I would have made offset_from return usize instead, as the earlier discussion mentioned, but it's too late for that now.

Another possibility may be count_from - it still retains a word from offset_from, so it shouldn't take more than a couple tries to discover through autocomplete, but it still communicates its unsignedness - an element count will/should always be unsigned, as opposed to a relative offset which can go in either direction. (Maybe also len_from.)

bors pushed a commit to rust-lang-ci/rust that referenced this issue May 29, 2024
This is an API that naturally should exist as a combination of byte_offset_from and sub_ptr
both existing (they showed up at similar times so this union was never made). Adding these
is a logical (and perhaps final) precondition of stabilizing ptr_sub_ptr (rust-lang#95892).
bors added a commit to rust-lang-ci/rust that referenced this issue May 29, 2024
feat(byte_sub_ptr): add ptr::byte_sub_ptr

This is an API that naturally should exist as a combination of byte_offset_from and sub_ptr both existing (they showed up at similar times so this union was never made). Adding these is a logical (and perhaps final) precondition of stabilizing ptr_sub_ptr (rust-lang#95892).
fmease added a commit to fmease/rust that referenced this issue May 30, 2024
feat(byte_sub_ptr): add ptr::byte_sub_ptr

This is an API that naturally should exist as a combination of byte_offset_from and sub_ptr both existing (they showed up at similar times so this union was never made). Adding these is a logical (and perhaps final) precondition of stabilizing ptr_sub_ptr (rust-lang#95892).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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

5 participants