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

give more help in the unaligned_references lint #91718

Merged
merged 1 commit into from
Dec 11, 2021

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 9, 2021

@rust-highfive
Copy link
Collaborator

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 9, 2021
@nagisa
Copy link
Member

nagisa commented Dec 9, 2021

Hm, from a wording perspective, just “copy the field contents” doesn't really help that much – copy to where, how, and in what way does this help is left for the reader to figure out. I assume what the meaning is here is “make a local variable with a copy of the field's contents and then take a reference to the variable”.

Acknowledging that, in an effort to produce reasonably looking diagnostics, we want to limit the length of any specific diagnostic string, I wonder if it we could have --explain mechanism work for future compatibility lints like these, thus giving more space to the non-parenthesized part of the description.


I'd also like some input from @estebank about addressing the reader. In the past they argued that messages of the form “consider…” are a poor style and “you can…” is probably not much different.

With both points in mind how about something along these lines:

= help: copy the field contents to a local variable, or replace the reference with a raw pointer and use read_unaligned/write_unaligned

@RalfJung
Copy link
Member Author

RalfJung commented Dec 9, 2021

I like that wording. However, I am a bit worried people will think 'oh so raw pointers are fine' and not realize that *p requires proper alignment. I know this makes the text rather long, but I still think I would prefer

= help: copy the field contents to a local variable, or replace the reference with a raw pointer and use read_unaligned/write_unaligned (loads and stores via *p must be properly aligned even when using raw pointers)

It's not like this is an extremely common warning, and when it arises then the programmer really needs to be aware of a lot of subtle details.

@estebank
Copy link
Contributor

estebank commented Dec 9, 2021

I like the final proposed output and agree that given how uncommon this will be in practice, going overboard with detail is better than being too terse.

@RalfJung RalfJung force-pushed the unaligned_references branch from 19f9284 to 7d18a45 Compare December 9, 2021 21:49
@nagisa
Copy link
Member

nagisa commented Dec 9, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 9, 2021

📌 Commit 7d18a45 has been approved by nagisa

@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 Dec 9, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 10, 2021
…gisa

give more help in the unaligned_references lint

Cc rust-lang#82523 (comment) `@kaisq`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2021
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91617 (Improve the readability of `List<T>`.)
 - rust-lang#91640 (Simplify collection of in-band lifetimes)
 - rust-lang#91682 (rustdoc: Show type layout for type aliases)
 - rust-lang#91711 (Improve `std::iter::zip` example)
 - rust-lang#91717 (Add deprecation warning for --passes)
 - rust-lang#91718 (give more help in the unaligned_references lint)
 - rust-lang#91782 (Correct since attribute for `is_symlink` feature)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bc0269d into rust-lang:master Dec 11, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 11, 2021
@RalfJung RalfJung deleted the unaligned_references branch December 11, 2021 23:38
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants