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

WaffleLapkin · 2022-08-20T22:39:44Z

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 · 2022-08-20T22:39:47Z

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

library/core/src/slice/sort.rs

WaffleLapkin · 2022-08-20T22:45:36Z

library/std/src/sys/sgx/abi/usercalls/tests.rs

            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]);


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

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...)

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

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

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.

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)

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

scottmcm · 2022-08-21T01:28:10Z

library/core/src/slice/sort.rs

-                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));


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).

scottmcm · 2022-08-21T01:36:08Z

library/panic_unwind/src/dwarf/eh.rs

    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);


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

scottmcm · 2022-08-21T01:41:43Z

@bors r+ rollup

bors · 2022-08-21T01:41:45Z

📌 Commit e4720e1 has been approved by scottmcm

It is now in the queue for this repository.

…rk, r=scottmcm Replace most uses of `pointer::offset` with `add` and `sub` 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_

…rk, r=scottmcm Replace most uses of `pointer::offset` with `add` and `sub` 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_

…rk, r=scottmcm Replace most uses of `pointer::offset` with `add` and `sub` 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_

…rk, r=scottmcm Replace most uses of `pointer::offset` with `add` and `sub` 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_

…rk, r=scottmcm Replace most uses of `pointer::offset` with `add` and `sub` 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_

…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#100556 (Clamp Function for f32 and f64) - rust-lang#100663 (Make slice::reverse const) - rust-lang#100697 ( Minor syntax and formatting update to doc comment on `find_vtable_types_for_unsizing`) - rust-lang#100760 (update test for LLVM change) - rust-lang#100761 (some general mir typeck cleanup) - rust-lang#100775 (rustdoc: Merge source code pages HTML elements together v2) - rust-lang#100813 (Add `/build-rust-analyzer/` to .gitignore) - rust-lang#100821 (Make some docs nicer wrt pointer offsets) - rust-lang#100822 (Replace most uses of `pointer::offset` with `add` and `sub`) - rust-lang#100839 (Make doc for stdin field of process consistent) - rust-lang#100842 (Add diagnostics lints to `rustc_transmute` module (zero diags)) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…rk, r=scottmcm Replace most uses of `pointer::offset` with `add` and `sub` 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_

…leftovers, r=scottmcm Some more cleanup in `core` - remove some integer casts from slice iter (proposed in rust-lang#100819 (comment)) - replace `as usize` casts with `usize::from` in slice sort (proposed in rust-lang#100822 (comment)) r? `@scottmcm`

…, r=scottmcm Some more cleanup in `core` - remove some integer casts from slice iter (proposed in rust-lang/rust#100819 (comment)) - replace `as usize` casts with `usize::from` in slice sort (proposed in rust-lang/rust#100822 (comment)) r? `@scottmcm`

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

e4720e1

rust-highfive assigned scottmcm Aug 20, 2022

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

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2022

WaffleLapkin commented Aug 20, 2022

View reviewed changes

WaffleLapkin mentioned this pull request Aug 20, 2022

Refactor some std code that works with pointer offstes #100823

Merged

scottmcm reviewed Aug 21, 2022

View reviewed changes

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

This was referenced Aug 21, 2022

Rollup of 12 pull requests #100828

Closed

Rollup of 12 pull requests #100829

Closed

matthiaskrgr mentioned this pull request Aug 21, 2022

Rollup of 11 pull requests #100837

Closed

matthiaskrgr mentioned this pull request Aug 21, 2022

Rollup of 11 pull requests #100840

Closed

This was referenced Aug 21, 2022

Rollup of 12 pull requests #100846

Closed

Rollup of 11 pull requests #100847

Merged

bors merged commit a45f69f into rust-lang:master Aug 21, 2022

rustbot added this to the 1.65.0 milestone Aug 21, 2022

WaffleLapkin deleted the no_offset_question_mark branch August 22, 2022 10:47

rust-timer mentioned this pull request Aug 26, 2022

Rollup of 8 pull requests #100977

Merged

WaffleLapkin mentioned this pull request Sep 4, 2022

Some more cleanup in core #101412

Merged

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

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

Uh oh!

Conversation

WaffleLapkin commented Aug 20, 2022

Uh oh!

rustbot commented Aug 20, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmcm commented Aug 21, 2022

Uh oh!

bors commented Aug 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

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