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

Make pointer related code in std and compiler nicer #100746

Closed
wants to merge 5 commits into from

Conversation

WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Aug 19, 2022

This PR makes code that works with pointer a little nicer. Namely it

  • Makes use of wrapping_byte_add, wrapping_byte_sub and byte_add (replacing explicit casts)
  • Makes use of is_aligned and is_aligned_to (replacing hand-written alignment checks)
  • Replaces pointer::offset with add and sub (generally making code cleaner)
  • If I did everything right, there are no behavior/semantic changes

r? @scottmcm

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 19, 2022
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 19, 2022
@rustbot

This comment was marked as resolved.

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

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 20, 2022

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

@scottmcm
Copy link
Member

  • If I did everything right, there are no behavior/semantic changes

You probably did, but this is a very long scroll bar.

Would you mind sending it as a couple of different PRs instead? (Especially since there's some conflicts now too, which is particularly likely with a big PR.)

For example, a PR that's just "hey, I replaced offset with add/sub in the way that's obviously correct" is something that I could easily scan and agree "yup, definitely" and sign-off on it quickly.

But once there's more complicated changes, like types of local variables, I feel like I should be thinking about it more before signing off. For example, in

-                let d1 = ((n % 100) as isize) << 1;
+                let d1 = ((n % 100) << 1) as usize;

you probably got it correct, but now that shift is possibly working in a different type, so it's not "obviously correct" -- it needs contextual analysis to be sure.

So separating those out into the lots-of-the-same-easy-change review and the fewer-more-careful-changes review would be a big help.

@rustbot author

@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 Aug 20, 2022
@WaffleLapkin
Copy link
Member Author

Sure, I'll split this up

@WaffleLapkin WaffleLapkin deleted the niiice_ptr branch August 20, 2022 21:23
@WaffleLapkin WaffleLapkin restored the niiice_ptr branch August 20, 2022 21:23
@WaffleLapkin WaffleLapkin deleted the niiice_ptr branch August 21, 2022 02:41
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 24, 2022
Refactor some `std` code that works with pointer offstes

This PR replaces `pointer::offset` in standard library with `pointer::add` and `pointer::sub`, [re]moving some casts and using `.addr()` while we are at it.

This is a more complicated refactor than all other sibling PRs, so take a closer look when reviewing, please 😃  (though I've checked this multiple times and it looks fine).

r? ``@scottmcm``

_split off from rust-lang#100746, continuation of #100822_
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 24, 2022
Refactor some `std` code that works with pointer offstes

This PR replaces `pointer::offset` in standard library with `pointer::add` and `pointer::sub`, [re]moving some casts and using `.addr()` while we are at it.

This is a more complicated refactor than all other sibling PRs, so take a closer look when reviewing, please 😃  (though I've checked this multiple times and it looks fine).

r? ```@scottmcm```

_split off from rust-lang#100746, continuation of #100822_
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 24, 2022
Refactor some `std` code that works with pointer offstes

This PR replaces `pointer::offset` in standard library with `pointer::add` and `pointer::sub`, [re]moving some casts and using `.addr()` while we are at it.

This is a more complicated refactor than all other sibling PRs, so take a closer look when reviewing, please 😃  (though I've checked this multiple times and it looks fine).

r? ````@scottmcm````

_split off from rust-lang#100746, continuation of #100822_
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-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.

6 participants