Skip to content

Clarify one-past-the-end pointer validity#154370

Closed
justanotheranonymoususer wants to merge 2 commits into
rust-lang:mainfrom
justanotheranonymoususer:clarify-ptr-add
Closed

Clarify one-past-the-end pointer validity#154370
justanotheranonymoususer wants to merge 2 commits into
rust-lang:mainfrom
justanotheranonymoususer:clarify-ptr-add

Conversation

@justanotheranonymoususer

Copy link
Copy Markdown
Contributor

It's documented that vec.as_ptr().add(vec.len()) is safe, but the main safety condition wasn't clear enough.

@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 Mar 25, 2026
@rustbot

rustbot commented Mar 25, 2026

Copy link
Copy Markdown
Collaborator

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @scottmcm, libs
  • @scottmcm, libs expanded to 8 candidates
  • Random selection from Mark-Simulacrum, scottmcm

Comment thread library/core/src/ptr/const_ptr.rs Outdated
///
/// * If the computed offset is non-zero, then `self` must be [derived from][crate::ptr#provenance] a pointer to some
/// [allocation], and the entire memory range between `self` and the result must be in
/// [allocation], and the entire memory range between `self` and the result (not including result) must be in

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

for sub the self argument is after the result, so this would need to be not including self, right? also "memory range from the result to self (exclusive)" reads a bit more fluently to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For sub, not sure if the note is relevant because the pointer isn't moving forward. If non-zero and non-wraparound, you can't end up in the one-past-the-end situation anyway.

For "not including result" vs "exclusive", for me as not a native speaker, my version is clearer, but I can change. Let me know.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"not including result" is missing an article: "not including the result". And that's quite verbose, so I too would prefer "exclusive".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed

@scottmcm

Copy link
Copy Markdown
Member

@rustbot reroll

@rustbot rustbot assigned joboet and unassigned scottmcm Mar 29, 2026
@justanotheranonymoususer

Copy link
Copy Markdown
Contributor Author

@joboet ping :) anything still needs to be done?

@joboet joboet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the delay, I needed a break from reviewing...

View changes since this review

Comment thread library/core/src/ptr/const_ptr.rs Outdated
///
/// * If the computed offset is non-zero, then `self` must be [derived from][crate::ptr#provenance] a pointer to some
/// [allocation], and the entire memory range between `self` and the result must be in
/// [allocation], and the entire memory range between `self` and the result (not including result) must be in

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"not including result" is missing an article: "not including the result". And that's quite verbose, so I too would prefer "exclusive".

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2026
@rustbot

rustbot commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 30, 2026
@rustbot

rustbot commented May 1, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Warning ⚠️

@justanotheranonymoususer

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 1, 2026
@joboet

joboet commented May 7, 2026

Copy link
Copy Markdown
Member

r? RalfJung

@rustbot rustbot assigned RalfJung and unassigned joboet May 7, 2026
@rustbot

rustbot commented May 7, 2026

Copy link
Copy Markdown
Collaborator

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

@RalfJung

RalfJung commented May 7, 2026

Copy link
Copy Markdown
Member

Cc @rust-lang/opsem

That's a good point -- ranges are by default left-inclusive right-exclusive in Rust, but this should be made explicit.
Would it be worth clarifying the left-inclusive part as well?


* If the computed offset is non-zero, then `self` must be [derived from][crate::ptr#provenance] a pointer to some
[allocation], and the entire memory range between `self` and the result must be in
[allocation], and the entire memory range between `self` and the result (exclusive) must be in

@RalfJung RalfJung May 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually this is wrong. If the offset is negative, it's inclusive the result and exclusive self. Or put differently, it's always min(self, result) .. max(self, result).

@rustbot author

View changes since the review

@rustbot rustbot 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 May 13, 2026
@justanotheranonymoususer

Copy link
Copy Markdown
Contributor Author

@RalfJung

Copy link
Copy Markdown
Member

That's fair, thanks for taking a stab! We should improve our docs here, but it's not easy to get such docs right.

I opened a PR at #156666.

@RalfJung RalfJung closed this May 17, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 17, 2026
@justanotheranonymoususer justanotheranonymoususer deleted the clarify-ptr-add branch May 26, 2026 20:04
jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 7, 2026
Clarify meaning of ranges in pointer offset docs

Supersedes rust-lang#154370
rust-timer added a commit that referenced this pull request Jun 8, 2026
Rollup merge of #156666 - RalfJung:offset-ranges, r=jhpratt

Clarify meaning of ranges in pointer offset docs

Supersedes #154370
github-actions Bot pushed a commit to rust-lang/stdarch that referenced this pull request Jun 8, 2026
Clarify meaning of ranges in pointer offset docs

Supersedes rust-lang/rust#154370
asukaminato0721 pushed a commit to asukaminato0721/rust-analyzer that referenced this pull request Jun 8, 2026
Clarify meaning of ranges in pointer offset docs

Supersedes rust-lang/rust#154370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants