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 a raw pointer iterator #91390

Closed
wants to merge 3 commits into from
Closed

Add a raw pointer iterator #91390

wants to merge 3 commits into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Nov 30, 2021

Context: the discussion under #91000, particularly scottmcm's comment #91000 (comment).

This PR adds core::ptr::IterConst<T> which is an iterator over *const T, and core::ptr::IterMut<T> which is an iterator over *mut T between a start and a one-past-the-end pointer. The IterMut naming matches the name of core::slice::IterMut, and the IterConst naming is novel but matches how raw pointers have const written in the type where elsewhere non-mut is only indicated by absence of a mut keyword.

Example:

let start: *const T = /*...*/;
let end: *const T = /*...*/;  // one past the end

for ptr in unsafe { ptr::IterConst::new(start, end) } {
    println!("{}", unsafe { *ptr });
}

The structure of this API with an unsafe-to-construct iterator type gives a clear place to put the proof obligation that requires the start and end to be inside the same allocated object, among other things. See #91000 (comment).

Future work:

  • Scottmcm brought up it might be convenient to have a counted constructor for these types, which takes *T + usize.

  • I've declared that passing start=null, end=null is UB for now. It's possible that this restriction can be eliminated but I wasn't able to work out whether the current implementation is well-defined in the case of null, or if null would need to be treated as a special case with branches. If special case, it's possible it's not worth supporting because of the performance penalty.

@rust-highfive
Copy link
Collaborator

r? @scottmcm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 30, 2021
@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 30, 2021
@CAD97
Copy link
Contributor

CAD97 commented Nov 30, 2021

It's worth considering if the slice iterators can/should be implemented in terms of the raw pointer iterators (though that doesn't necessarily have to happen in this PR).

Along those lines, I prefer slice::RawIter[Mut] to ptr::Iter[Const|Mut], since a pointer span is effectively a raw slice.

I also think it makes sense to attach some of this functionality to *const [T]; the *T + usize is logically a ptr::slice_from_raw_parts and unsafe into iterator. Especially since a slice is pointer + count.

(Not arguing against direct iter constructors, just that *[T] is a part of the family here.)

And one more note: we need to be careful to specify the behavior for ZSTs. There are three options as I understand it:

  • disallow use with ZST pointees, and declare such as library UB / contract violation;
  • specify ZSTs with stride zero, meaning begin==end means none, otherwise infinitely many; or
  • specify and guarantee the current behavior utilized by slices (or some other similar scheme) where ZSTs can be iterated.

Again, not necessarily something that needs to be solved for this PR, but should be considered before stabilization.

--> $DIR/ptr-range.rs:3:14
|
LL | for _ in range {}
| ^^^^^ use core::ptr::IterConst to iterate a range of pointers
Copy link
Member

Choose a reason for hiding this comment

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

Very nice! ❤️

/// only dereferenceable if the `start` and `end` with which the
/// iterator was constructed both point into the same allocated object.
/// Dereferencing a pointer obtained via iteration when this is not the
/// case is Undefined Behavior.
Copy link
Member

Choose a reason for hiding this comment

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

I think this paragraph could use some tweaking. new now requires the same allocated object (explicitly in safety text and indirectly via talking about being equivalent to looping via offset(1)), so I think the "same object" UB has already been hit by the iteration before the dereference comes into play now.

(Even iterconst.next().unwrap() has run an offset(1), even if you only ever look at the first pointer.)

/// * Both pointers must be *derived from* a pointer to the same
/// object. (See [pointer::offset_from] for an example.)
///
/// * `null::<T>() < start <= end`.
Copy link
Member

Choose a reason for hiding this comment

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

minor nit: potentially this should be null_mut in the mut version?

@scottmcm
Copy link
Member

scottmcm commented Nov 30, 2021

Conceptually I agree that doing slice iterators via these would make sense, but those are so tightly optimized because of how critical they are for performance (to the extent that they use macros instead of inline functions because it made a difference) that I think it'd be better to leave the slice iterators alone.

On the ZST question: I think it'd be better to block them. Importantly, that would allow these iterators to be ExactSizeIterator in addition to TrustedLen, and I can absolutely see people wanting the .len() on these that that would provide. And I think the invariants you wrote for these types match exactly what you need to call offset_from in order to get that length. EDIT: Also, C++ doesn't have true ZSTs (insert nuance here about EBC and such) so not supporting them could also be considered removing a footgun. That could be done by assert!ing, so that it's not a safety violation, since it wouldn't add a runtime check.

But overall, I like this. And it's all unstable, so I'd be happy to r+ it with a tracking issue. (And continue various conversations, like null or optimizations with unchecked_sub, over there.)

Aside: I'm amused that highfive picked me as reviewer for this PR 🙂

@dtolnay dtolnay 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 Dec 1, 2021
@bors
Copy link
Contributor

bors commented Dec 2, 2021

☔ The latest upstream changes (presumably #91455) made this pull request unmergeable. Please resolve the merge conflicts.

($ty:ident, *$mutability:tt T) => {
/// Iterator with items generated by incrementing a raw pointer.
///
/// # Safety
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, we should note that T must not be ZST because in that case it is very easy to make invalid iterations.

@dtolnay
Copy link
Member Author

dtolnay commented Jan 22, 2022

Revisiting this somewhat later, I think I would probably not use these in my own code if this existed. let mut p = start; while p < end { …; p = p.offset(1) } is gonna be good enough. for p in start..end is something I would use if it worked but that has issues.

@dtolnay dtolnay closed this Jan 22, 2022
@dtolnay dtolnay deleted the ptriter branch January 22, 2022 04:51
@Lokathor
Copy link
Contributor

One thing I have bothered to make an iterator in my own code is a "zptr iter", which takes a pointer to a zero-terminated sequence and then does the iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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