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

Replace most uses of pointer::offset with add and sub #100822

Merged
merged 1 commit into from
Aug 21, 2022

Conversation

WaffleLapkin
Copy link
Member

As PR title says, it replaces pointer::offset in compiler and standard library with pointer::add and pointer::sub. This generally makes code cleaner, easier to grasp and removes (or, well, hides) integer casts.

This is generally trivially correct, .offset(-constant) is just .sub(constant), .offset(usized as isize) is just .add(usized), etc. However in some cases we need to be careful with signs of things.

r? @scottmcm

split off from #100746

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 20, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 20, 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

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2022
library/core/src/slice/sort.rs Show resolved Hide resolved

// Verify copy
for byte in 0..size {
unsafe {
assert_eq!(*dst.as_ptr().offset(offset + byte as isize), src[byte as usize]);
assert_eq!(*dst.as_ptr().add(offset + byte), src[byte as usize]);
Copy link
Member Author

Choose a reason for hiding this comment

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

sneaky: offset was {integer} previously inferred to isize, now to usize.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like byte was previously i32 (because unconstrained, as it was always ased) and is now usize? So it can probably be src[byte] too.

(It would be nice to have a warning for T-to-T as casts...)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about linting T2T casts too, while doing these refactorings. It should probably be easy to add 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, there is a clippy lint for that (docs, source), should I open an uplifting issue/pr?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the problem is if you want to keep it -- say it's there to be resilient to c_int widths or something, where it's c_int as i64, which might be a nop sometimes.

For a clippy lint saying "hey, just allow it" is fine, but for rustc lints we generally want to be able to give a "or write ______ instead to make it obvious why you want to keep it like this" suggestion.

Nothing jumped to mind to me for how to do that, but hopefully you'll come up with something clever to lint only in the valuable places.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not linting i64 as c_int is probably possible by recording in hir (?) that c_int is a type alias, but not linting c_int as i64 I think it impossible, there is simply no way to differentiate a variable of type c_int with a variable of type i64 I believe (if c_int is an alias to i64)

Copy link
Member Author

Choose a reason for hiding this comment

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

As a second thought, would this still be useful as a allow-by-default lint? 🤔

ptr::swap(l.offset(*end_l as isize), r.offset(-1));
r = r.offset(-1);
end_l = end_l.sub(1);
ptr::swap(l.add(*end_l as usize), r.sub(1));
Copy link
Member

Choose a reason for hiding this comment

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

Not this PR, but consider following along to use usize::from(*end_l) here -- I went off to check that *end_l wasn't signed, and if it was a from then it'd be obvious that it's unsigned (since the from wouldn't compile for signed -> unsigned).

@@ -75,7 +75,7 @@ pub unsafe fn find_eh_action(lsda: *const u8, context: &EHContext<'_>) -> Result

let call_site_encoding = reader.read::<u8>();
let call_site_table_length = reader.read_uleb128();
let action_table = reader.ptr.offset(call_site_table_length as isize);
let action_table = reader.ptr.add(call_site_table_length as usize);
Copy link
Member

Choose a reason for hiding this comment

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

Not this PR's problem, but it's interesting that this reads a u64 and then offsets it without any checking.

@scottmcm
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 21, 2022

📌 Commit e4720e1 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 Aug 21, 2022
@bors bors merged commit a45f69f into rust-lang:master Aug 21, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 21, 2022
@WaffleLapkin WaffleLapkin deleted the no_offset_question_mark branch August 22, 2022 10:47
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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.

5 participants