Skip to content

Clarify unaligned fields in ptr::{read,write}_unaligned - #62323

Merged
bors merged 2 commits into
rust-lang:masterfrom
Centril:clarify-read-unaligned
Jul 5, 2019
Merged

Clarify unaligned fields in ptr::{read,write}_unaligned#62323
bors merged 2 commits into
rust-lang:masterfrom
Centril:clarify-read-unaligned

Conversation

@Centril

@CentrilCentril commented Jul 3, 2019

Copy link
Copy Markdown
Contributor

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2019
@RalfJung

RalfJung commented Jul 3, 2019

Copy link
Copy Markdown
Member

write_unaligned contains a similar example, you probably want to do the same there.

RalfJung
RalfJung reviewed Jul 3, 2019
View reviewed changes
Comment threadsrc/libcore/ptr/mod.rs Outdated
RalfJung
RalfJung reviewed Jul 3, 2019
View reviewed changes
Comment threadsrc/libcore/ptr/mod.rs Outdated
@Centril
Centrilforce-pushed the clarify-read-unaligned branch from 1fb34b7 to de9297eCompareJuly 3, 2019 23:31
@CentrilCentril mentioned this pull request Jul 4, 2019
Closed
@CentrilCentril changed the title Clarify unaligned fields in ptr::read_unalignedClarify unaligned fields in ptr::{read,write}_unalignedJul 4, 2019
@Centril
Centrilforce-pushed the clarify-read-unaligned branch from de9297e to b156356CompareJuly 4, 2019 03:31
@Centril
Centrilforce-pushed the clarify-read-unaligned branch from b156356 to bee964cCompareJuly 4, 2019 03:32
@RalfJung

RalfJung commented Jul 4, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Please don't force-push while review is still in progress, unless necessary for a rebase. (a) force-pushes don't trigger email notifications, and (b) that makes it harder to figure out what changed since the last review. (I am aware that GitHub shows those diffs, but e.g. it does not show the full diff when you do multiple force-pushes in a row.)

rust-highfive even says this when welcoming new users, but I guess it's been a while since you got that message. ;)

RalfJung
RalfJung reviewed Jul 4, 2019
View reviewed changes
Comment threadsrc/libcore/ptr/mod.rs Outdated
RalfJung
RalfJung reviewed Jul 4, 2019
View reviewed changes
Comment threadsrc/libcore/ptr/mod.rs Outdated
@Centril

Centril commented Jul 4, 2019

Copy link
Copy Markdown
ContributorAuthor

(a) force-pushes don't trigger email notifications,

No, but a comment in thread does, which I did make... ;)

(b) that makes it harder to figure out what changed since the last review. (I am aware that GitHub shows those diffs, but e.g. it does not show the full diff when you do multiple force-pushes in a row.)

It does, you can click on each "force-push" link in sequence and it will show the relative diff. I am aware that force pushing makes it harder to figure out what changed since last review when the diff and PR is large (and then I wouldn't do it) but in this case it's small, so a cleaner history is imo to be preferred.

@RalfJung

RalfJung commented Jul 4, 2019

Copy link
Copy Markdown
Member

It does, you can click on each "force-push" link in sequence and it will show the relative diff.

That is incorrect. When you do two force-pushes in a row with no other event in between, GH will just say "force-pushed twice, most recently from X to Y", with no way to get the full diff.

a cleaner history is imo to be preferred.

You can squash after I am done with reviewing. But please don't while review is still in progress. Please follow the process we also expect new contributors to follow.

Centril
Centril commented Jul 4, 2019
View reviewed changes
Comment threadsrc/libcore/ptr/mod.rs Outdated
Centril
Centril commented Jul 4, 2019
View reviewed changes
Comment threadsrc/libcore/ptr/mod.rs Outdated
@RalfJung

RalfJung commented Jul 4, 2019

Copy link
Copy Markdown
Member

Thanks a lot! r=me, after squashing if you want.

@Centril

Centril commented Jul 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Too much going on today... so I'll just...

@bors r=RalfJung rollup

@bors

bors commented Jul 4, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 54527db has been approved by RalfJung

@borsbors 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 Jul 4, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 4, 2019
@Mark-Simulacrum
Rollup merge ofrust-lang#62323- Centril:clarify-read-unaligned, r=R…
cabf8b0
…alfJung
Clarify unaligned fields in ptr::{read,write}_unaligned
r? @RalfJung
@Mark-SimulacrumMark-Simulacrum mentioned this pull request Jul 4, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
@Centril
Rollup merge ofrust-lang#62323- Centril:clarify-read-unaligned, r=R…
d81398c
…alfJung
Clarify unaligned fields in ptr::{read,write}_unaligned
r? @RalfJung
@CentrilCentril mentioned this pull request Jul 5, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
@Centril
Rollup merge ofrust-lang#62323- Centril:clarify-read-unaligned, r=R…
d3569dd
…alfJung
Clarify unaligned fields in ptr::{read,write}_unaligned
r? @RalfJung
@CentrilCentril mentioned this pull request Jul 5, 2019
Merged
bors added a commit that referenced this pull request Jul 5, 2019
@bors
Auto merge of#62407- Centril:rollup-g0zmff7, r=Centril
853f300
Rollup of 10 pull requests
Successful merges:
- #62123 ( Remove needless lifetimes (std))
- #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.)
- #62169 (Derive which queries to save using the proc macro)
- #62238 (Fix code block information icon position)
- #62292 (Move `async || ...` closures into `#![feature(async_closure)]`)
- #62323 (Clarify unaligned fields in ptr::{read,write}_unaligned)
- #62324 (Reduce reliance on `await!(...)` macro)
- #62371 (Add tracking issue for Box::into_pin)
- #62383 (Improve error span for async type inference error)
- #62388 (Break out of the correct number of scopes in loops)
Failed merges:
r? @ghost
@bors
bors merged commit 54527db into rust-lang:masterJul 5, 2019
@Centril
Centril deleted the clarify-read-unaligned branch July 5, 2019 16:15
@FreyskeydFreyskeyd mentioned this pull request Jul 8, 2019
Merged
Centril added a commit to Centril/rust that referenced this pull request Jul 10, 2019
@Centril
Rollup merge ofrust-lang#62493- Freyskeyd:valid_example_read-write_…
d0cec04
…unaligned, r=rkruppe
rust-lang#62357: doc(ptr): add example for {read,write}_unaligned
related to rust-lang#62357
> With rust-lang#62323 the only example (that had UB and was thus invalid) in std::ptr::read_unaligned and std::ptr::write_unaligned is removed.
> We should add a valid example of using the aforementioned functions.
Signed-off-by: Freyskeyd <simon.paitrault@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJungRalfJungRalfJung left review comments

Assignees

@RalfJungRalfJung

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Centril@RalfJung@bors@rust-highfive