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

Add allow(unused_unsafe) to some functions. #1294

Merged
merged 1 commit into from
Sep 19, 2020

Conversation

vi
Copy link
Contributor

@vi vi commented Sep 12, 2020

No description provided.

@vi vi force-pushed the overstrict_deny_unused branch from a401dd2 to df9b44e Compare September 12, 2020 10:54
@vi
Copy link
Contributor Author

vi commented Sep 12, 2020

Or shall a bunch of unsafe{} blocks be removed instead? Will it cause incompatibilities for users with legacy rustc or deps?

@vi vi marked this pull request as ready for review September 12, 2020 11:02
src/lib.rs Outdated
@@ -11,6 +11,7 @@
#![cfg_attr(test, deny(warnings))]
#![recursion_limit = "500"]
#![deny(unused)]
#![allow(unused_unsafe)]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these unused unsafe blocks? Can't you add the lint more locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect it to be this.

Shall it be fixed just for for this use case or also for possible future unsafe-to-safe conversions?

Shall it be fixed by just removing unsafe{} or by allowing unused_unsafe selectively?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do it selectively, just where we need it. And BTW we'll have to make a 0.18.1 release just for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a point release? Shouldn't lint caps hide those errors when nix is used as a crates.io dependency?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are "lint caps"?

Copy link

@RalfJung RalfJung Sep 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cargo builds dependencies by calling rustc --cap-lints allow. This means lints are turned off when building dependencies, so crates work with newer rustc even if they use deny(warnings) (which would otherwise be a really bad idea, it would basically make it impossible to ever add new lints to rustc).

src/lib.rs Outdated
@@ -11,6 +11,7 @@
#![cfg_attr(test, deny(warnings))]
#![recursion_limit = "500"]
#![deny(unused)]
#![allow(unused_unsafe)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do it selectively, just where we need it. And BTW we'll have to make a 0.18.1 release just for this change.

(rust-lang/libc#1870)

Denied lint being overridden instead of unsafe block removed
to preserve compatibility with old `libc` versions.
@vi vi force-pushed the overstrict_deny_unused branch from df9b44e to 4db701d Compare September 13, 2020 16:55
@vi vi changed the title Relax overly strict deny(unused) policy. Add allow(unused_unsafe) to some functions. Sep 13, 2020
@vi
Copy link
Contributor Author

vi commented Sep 13, 2020

Edited the pull request.

Is this this one still needed in face of #1281 which includes similar (but somewhat conflicting) changes?

@vi
Copy link
Contributor Author

vi commented Sep 18, 2020

@asomers What more to be done for this?

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 19, 2020

Build succeeded:

@bors bors bot merged commit 2c24405 into nix-rust:master Sep 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants