Skip to content

Fix most warnings in tests #26626

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

Closed
wants to merge 1 commit into from

Conversation

cristicbz
Copy link
Contributor

This PR attempts to fix most warnings spewed by running make check:

  • Adds several #[allow(deprecated)] attributes to tests which test deprecated code: unfoldr, BufStream, RandomAccessIterator, reverse_in_place, thread::scoped.
  • Adds underscores to a bunch of unused args and locals.
  • Adds #[cfg(not(test))] to places like primitive inherent impls where, AFAIU, where at test time the impls themselves are not requried since they are already provied by the libstd crate against which we link.
  • Removes several unused imports from a bunch of places.
  • Replaces Thunk to FnBox in remutex.rs and gets rid of thread::scoped.

After these changes, I still get some warnings, which I find really strange:

  • In sync::future the tests throw deprecation warnings despite the toplevel module attribute allowing deprecation. Weird.
  • I get a warning with the text for the Future deprecation, but target at libstd/lib.rs line 1 column 1:
src/libstd/lib.rs:1:1: 1:1 warning: use of deprecated item: implementation does not match the quality of the standard library and this will likely be prototyped outside in crates.io first, #[warn(deprecated)] on by default
src/libstd/lib.rs:1 // Copyright 2012-2014 The Rust Project Developers. See the COPYRIGHT
  • Unknown feature for definitely known feature. Is this some kind of weird artifact of how staging works? Would some kind of #[cfg(not(test))] for the feature be helpful?
src/libstd/lib.rs:118:12: 118:22 warning: unused or unknown feature, #[warn(unused_features)] on by default
src/libstd/lib.rs:118 #![feature(core_float)]
                                 ^~~~~~~~~~
  • Unused warnings for things which are definitely used:
src/libstd/sys/unix/stack_overflow.rs:64:5: 64:37 warning: static item is never used: `PAGE_SIZE`, #[warn(dead_code)] on by default
src/libstd/sys/unix/stack_overflow.rs:64     static mut PAGE_SIZE: usize = 0;
                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/libstd/sys/unix/stack_overflow.rs:67:5: 95:6 warning: function is never used: `signal_handler`, #[warn(dead_code)] on by default
src/libstd/sys/unix/stack_overflow.rs:67     unsafe extern fn signal_handler(signum: libc::c_int,
src/libstd/sys/unix/stack_overflow.rs:68                                      info: *mut siginfo,
src/libstd/sys/unix/stack_overflow.rs:69                                      _data: *mut libc::c_void) {
src/libstd/sys/unix/stack_overflow.rs:70
src/libstd/sys/unix/stack_overflow.rs:71         // We can not return from a SIGSEGV or SIGBUS signal.
src/libstd/sys/unix/stack_overflow.rs:72         // See: https://www.gnu.org/software/libc/manual/html_node/Handler-Returns.html
                                         ...
src/libstd/sys/unix/stack_overflow.rs:74:9: 80:10 warning: function is never used: `term`, #[warn(dead_code)] on by default
src/libstd/sys/unix/stack_overflow.rs:74         unsafe fn term(signum: libc::c_int) -> ! {
src/libstd/sys/unix/stack_overflow.rs:75             use core::mem::transmute;
src/libstd/sys/unix/stack_overflow.rs:76
src/libstd/sys/unix/stack_overflow.rs:77             signal(signum, transmute(SIG_DFL));
src/libstd/sys/unix/stack_overflow.rs:78             raise(signum);
src/libstd/sys/unix/stack_overflow.rs:79             intrinsics::abort();
                                         ...
src/libstd/sys/unix/stack_overflow.rs:99:5: 116:6 warning: function is never used: `init`, #[warn(dead_code)] on by default
src/libstd/sys/unix/stack_overflow.rs:99     pub unsafe fn init() {
src/libstd/sys/unix/stack_overflow.rs:100         let psize = libc::sysconf(libc::consts::os::sysconf::_SC_PAGESIZE);
src/libstd/sys/unix/stack_overflow.rs:101         if psize == -1 {
src/libstd/sys/unix/stack_overflow.rs:102             panic!("failed to get page size");
src/libstd/sys/unix/stack_overflow.rs:103         }
src/libstd/sys/unix/stack_overflow.rs:104

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

@bors: r+ 2106e003f7adf403178be9eb41be2b8e7ad57961

Certainly a great step forward, thanks!

Would some kind of #[cfg(not(test))] for the feature be helpful?

You should be able to add #[cfg_attr(not(test), feature(...))]

@bors
Copy link
Collaborator

bors commented Jun 27, 2015

⌛ Testing commit 2106e00 with merge e5a9d69...

@bors
Copy link
Collaborator

bors commented Jun 27, 2015

💔 Test failed - auto-mac-64-opt

@cristicbz cristicbz force-pushed the fix-test-warnings branch from 2106e00 to 257212a Compare June 27, 2015 22:32
@cristicbz
Copy link
Contributor Author

Silly leftover issue in the iter.rs integration test. Though I swear I did run a full make check before---doesn't make check do all stages?

@bors: retry 257212a

@cristicbz
Copy link
Contributor Author

@bors: try

@alexcrichton
Copy link
Member

@bors: r+ 257212a

Perhaps it's a platform-specific issue?

@bors
Copy link
Collaborator

bors commented Jun 28, 2015

⌛ Testing commit 257212a with merge a497482...

@bors
Copy link
Collaborator

bors commented Jun 28, 2015

💔 Test failed - auto-win-gnu-64-opt

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Collaborator

bors commented Jun 28, 2015

⌛ Testing commit 257212a with merge 150358a...

@bors
Copy link
Collaborator

bors commented Jun 28, 2015

💔 Test failed - auto-linux-64-x-android-t

@bors
Copy link
Collaborator

bors commented Jun 28, 2015

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

@huonw
Copy link
Member

huonw commented Jul 7, 2015

Hey @cristicbz, this seems to need a rebase. 😄

@cristicbz
Copy link
Contributor Author

Ah yes, sorry this fell through the cracks. I'm struggling a bit to repro the failed tests though. Is running make check enough? It seems like the buildbots trigger things I don't.

@huonw
Copy link
Member

huonw commented Jul 7, 2015

Don't worry! It feel through my cracks too.

The build-bots do make check, but they run on more platforms (windows, mac, linux, ...) and configurations.

@Gankra
Copy link
Contributor

Gankra commented Jul 27, 2015

@cristicbz feel free to push up changes you think work and we can run them on our try servers for you.

@steveklabnik
Copy link
Member

@cristicbz it's been a long time, are you still interested in fixing up this PR, or should we close it?

@cristicbz
Copy link
Contributor Author

Sorry about this, I'll close it. I might have a new, more focused go when I get more time, I've just been utterly swamped for the last few months or so :(

@cristicbz cristicbz closed this Sep 27, 2015
@steveklabnik
Copy link
Member

Completely understood, happens to us all. Good luck with things!

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.

7 participants