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

rust: globally warn on unsafe_op_in_unsafe_fn #339

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

ojeda
Copy link
Member

@ojeda ojeda commented Jun 3, 2021

This includes non-rust/ code.

As soon as the warnings are cleaned up, we can move it to a hard error.

Fixes: #285

Signed-off-by: Miguel Ojeda [email protected]

This includes non-`rust/` code.

As soon as the warnings are cleaned up, we can move it to a hard error.

Fixes: Rust-for-Linux#285

Signed-off-by: Miguel Ojeda <[email protected]>
@ksquirrel
Copy link
Member

Review of 34658e643210:

  • ✔️ Commit 34658e6: Looks fine!

@ojeda ojeda merged commit 0b9bcdf into Rust-for-Linux:rust Jun 3, 2021
@ojeda ojeda deleted the unsafe branch June 3, 2021 21:38
@alex
Copy link
Member

alex commented Jun 3, 2021

Let's get a bug to flip it to an error after we've got the warning count to 0.

@ojeda
Copy link
Member Author

ojeda commented Jun 3, 2021

Let's get a bug to flip it to an error after we've got the warning count to 0.

+1 I'm on it.

@TheSven73
Copy link
Collaborator

Was it such a great idea to merge this in its current form? I am getting tons of build warnings now.

@TheSven73
Copy link
Collaborator

warning: 118 warnings emitted
Do we divide up the work between a few volunteers? :)

@nbdd0121
Copy link
Member

nbdd0121 commented Jun 3, 2021

Well, we need to merge it; unsafe_op_in_unsafe_fn is not a conventional warnings. If you allow it, it will warn about using unsafe in unsafe fn; otherwise if warns about not using unsafe in unsafe fn.

So if we don't merge this we'll have to use #[allow(unused_unsafe)] while doing migration, which means that we won't have warning for either style :)

@TheSven73
Copy link
Collaborator

Not disputing that we should merge it... just fell off my chair when I saw the gazillion warnings :)

So do we divvy up these 118 warnings between volunteers? I can help if required.

@ojeda
Copy link
Member Author

ojeda commented Jun 3, 2021

Yes please! Follow the discussion in #340.

@ojeda
Copy link
Member Author

ojeda commented Jun 3, 2021

Was it such a great idea to merge this in its current form? I am getting tons of build warnings now.

Like Gary explained, it was either that or adding allow everywhere, which is not nice and would create more churn.

If we were in mainline, I would have gone the other way -- but since it is just "us"... better to push everyone to fix them ;)

Not disputing that we should merge it... just fell off my chair when I saw the gazillion warnings :)

Yeah, that is why I tagged it as prio: high -- hopefully in a week or so we have them sorted out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Consider opting for unsafe_op_in_unsafe_fn
5 participants