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 ptr::offset_to #40943

Merged
merged 2 commits into from
Apr 6, 2017
Merged

Add ptr::offset_to #40943

merged 2 commits into from
Apr 6, 2017

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Mar 31, 2017

This PR adds a method to calculate the signed distance (in number of elements) between two pointers. The resulting value can then be passed to offset to get one pointer from the other. This is similar to pointer subtraction in C/C++.

There are 2 special cases:

  • If the distance is not a multiple of the element size then the result is rounded towards zero. (in C/C++ this is UB)
  • ZST return None, while normal types return Some(isize). This forces the user to handle the ZST case in unsafe code. (C/C++ doesn't have ZSTs)

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

/// ```
#[unstable(feature = "offset_to", issue = "0")]
#[inline]
pub fn offset_to(self, other: *const T) -> isize where T: Sized {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be *mut T?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it doesn't really matter whether the pointer is mutable or not, and *const T accepts both.

@alexcrichton
Copy link
Member

I'm somewhat uneasy about the ZST semantics here, just because we chose one convention in slices doesn't mean that the same convention is guaranteed to be used elsewhere. The fact that they're counted by 1 isn't exposed in the slice api I believe?

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 1, 2017
@Amanieu
Copy link
Member Author

Amanieu commented Apr 1, 2017

Well I don't really see any other good solutions for ZSTs.

Possible alternatives:

  • Return 0
  • Panic
  • UB (but then we would have to make the function unsafe)

@ranma42
Copy link
Contributor

ranma42 commented Apr 1, 2017

In fact the convention for slices is not respected for the offset intrinsic.
Personally, I would love if offset_of could give a compile-time error if used with ZST; if this is considered unacceptable, among the proposed options I would definitely prefer a panic.

@bluss
Copy link
Member

bluss commented Apr 1, 2017

It could return an Option, with None for ZST. It forces all users to consider the case, and they may for example use it as an assertion: They simply don't support ZST and unfortunately have no other way to restrict types.

@alexcrichton Yeah you're right, the slice iterators don't expose this, it's part of their inner workings. What this PR doesn't show is that libcore already has this exact function internally, and it makes sense to consider the slice iterator's ptrdistance, to see if it is something that should be generally available. It often is exactly the tools we need that others will need, isn't it? (Edit: Hm, this PR does show that. Maybe I didn't see it or PR was different before).

I would like use regular arithmetic for the non-ZST case (debug assertion if the end is before the start). Is it common that you have two pointers and no idea which one is first?

@bluss
Copy link
Member

bluss commented Apr 1, 2017

Another usage site of the iterator difference. Just to make note.

fn size_hint(&self) -> (usize, Option<usize>) {

@Amanieu
Copy link
Member Author

Amanieu commented Apr 1, 2017

I would like use regular arithmetic for the non-ZST case (debug assertion if the end is before the start). Is it common that you have two pointers and no idea which one is first?

I disagree, the returned value is an isize, which matches the argument for the offset method. Since offset can go both forward and backward, it makes sense to allow the second pointer to be before the first.

@Amanieu Amanieu force-pushed the offset_to branch 6 times, most recently from 4615e73 to c01b6ef Compare April 2, 2017 15:14
@Amanieu
Copy link
Member Author

Amanieu commented Apr 2, 2017

I changed the function to return an Option<usize> instead.

diff / (if size == 0 { 1 } else { size })
match start.offset_to(end) {
Some(x) => x as usize,
None => end as usize - start as usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a wrapping_sub, otherwise it might underflow.

Copy link
Member Author

@Amanieu Amanieu Apr 2, 2017

Choose a reason for hiding this comment

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

end should always be greater than or equal to start, so no overflow is possible.

Copy link
Member

@bluss bluss Apr 2, 2017

Choose a reason for hiding this comment

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

For the ZST case the pointers are not assumed to be non-null, presumably because the wrapping arithmetic offset on their pointers can make them wrap around to zero. (There's also no NonZero around the pointer in the iterator).

In the current formulation the ZST iteration always starts at 0x1. The maximum ZST vec length should be usize::MAX! Which brings the end pointer to wrap around. I had to experiment to see for myself.

fn main() {
    unsafe {
        let v = vec![(); !0];
        println!("{:?}", v.iter().len());
        let iter_repr: [usize; 2] = std::mem::transmute(v.iter());
        println!("{:?}", iter_repr);
    }
}

Output of the program:

18446744073709551615
[1, 0]

So for ZST it does wrap around.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, my bad, I was under the impression that the maximum vector size was isize::MAX, not usize::MAX.

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, the existing vec::IntoIter::size_hint implementation does not use wrapping arithmetic and can panic in the example you showed.

Should I add a test for this?

Copy link
Member

Choose a reason for hiding this comment

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

I wouln't really bother with it. Without optimizations, the !0 length vector busy-loops when creating and dropping it, which is tedious. Either way, the debug assertion for the arithmetic does not show up in a small test. I'm still not clear of the rules for arithmetic assertions from libstd, but I don't think this one reaches users.

@Amanieu Amanieu force-pushed the offset_to branch 2 times, most recently from dadee88 to 11bdf60 Compare April 2, 2017 23:44
@Amanieu
Copy link
Member Author

Amanieu commented Apr 3, 2017

All the tests pass, this PR is ready for review. A tracking issue will need to be added.

@alexcrichton
Copy link
Member

I like the Option idea! Want to file an issue for this and I'll r+?

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 5, 2017

📌 Commit 1f70247 has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 5, 2017
Add ptr::offset_to

This PR adds a method to calculate the signed distance (in number of elements) between two pointers. The resulting value can then be passed to `offset` to get one pointer from the other. This is similar to pointer subtraction in C/C++.

There are 2 special cases:

- If the distance is not a multiple of the element size then the result is rounded towards zero. (in C/C++ this is UB)
-  ZST return `None`, while normal types return `Some(isize)`. This forces the user to handle the ZST case in unsafe code. (C/C++ doesn't have ZSTs)
arielb1 pushed a commit to arielb1/rust that referenced this pull request Apr 5, 2017
Add ptr::offset_to

This PR adds a method to calculate the signed distance (in number of elements) between two pointers. The resulting value can then be passed to `offset` to get one pointer from the other. This is similar to pointer subtraction in C/C++.

There are 2 special cases:

- If the distance is not a multiple of the element size then the result is rounded towards zero. (in C/C++ this is UB)
-  ZST return `None`, while normal types return `Some(isize)`. This forces the user to handle the ZST case in unsafe code. (C/C++ doesn't have ZSTs)
bors added a commit that referenced this pull request Apr 6, 2017
Rollup of 12 pull requests

- Successful merges: #40479, #40561, #40709, #40815, #40909, #40927, #40943, #41015, #41028, #41052, #41054, #41065
- Failed merges:
@bors bors merged commit 1f70247 into rust-lang:master Apr 6, 2017
@kornelski
Copy link
Contributor

I've just tried to use this function, and found that the order of arguments is backwards compared to C.

Since that method is probably mostly for translating C code, could it be changed to be offset_from with args reversed?

#include <stdio.h>

int main() {
    char a[10];

    printf("%ld\n", &a[5] - a); // 5
}
#![feature(offset_to)]

fn main() {
    let a = [0u8;10];
    
    println!("{:?}", a[5..].as_ptr().offset_to(a.as_ptr())); // -5
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants