Fix compiler warnings for unused code when compiling without decoder #834
Conversation
roderickvd
left a comment
roderickvd
left a comment
There was a problem hiding this comment.
Some ideas to make it more idiomatic and convey clearer intent.
| #[cfg_attr(docsrs, doc(cfg(feature = "dither")))] | ||
| #[inline] | ||
| fn dither(self, target_bits: BitDepth, algorithm: DitherAlgorithm) -> Dither<Self> | ||
| fn dither(self, target_bits: crate::BitDepth, algorithm: DitherAlgorithm) -> Dither<Self> |
There was a problem hiding this comment.
What is your rationale here?
There was a problem hiding this comment.
If you import crate::BitDepth and then just use BitDepth you'll get a warning about the unused import when the dither feature is not enabled.
We can also put the import behind a feature flag, but then we end up with feature-gated stuff spread more out and it's just easier to accidentally mess up.
| Err(DecoderError::UnrecognizedFormat) | ||
| { | ||
| let _ = data; | ||
| Err(DecoderError::UnrecognizedFormat) |
There was a problem hiding this comment.
Tagging #[allow(unused_variables)] may convey intent more clearly.
There was a problem hiding this comment.
I don't think so. It would just allow unused variables (generally) and I think it would need to go on the function scope, and you'd want to only disable the lint for not(feature = "symphonia").
In my opinion the last block - whether symphonia is enabled, or not - should use the data somehow, and that's what this does.
It's late and I think I'm failing to describe why I think using let _ = data; is better. Maybe tomorrow it will be clearer for either of us.
| DecoderImpl::Symphonia(source, PhantomData) => source.try_seek(pos), | ||
| DecoderImpl::None(_, _) => unreachable!(), | ||
| DecoderImpl::None(_, _) => { | ||
| let _ = pos; |
There was a problem hiding this comment.
Trick here is to make the argument _pos in the function signature.
There was a problem hiding this comment.
Starting a variable with underscore is used for unused variables. pos is not generally unused.
There was a problem hiding this comment.
You can use it as _pos too: source.try_seek(_pos).
There was a problem hiding this comment.
Yes, but naming it _pos communicates the intent of it being unused - which it is not generally.
| let sample = source.next(); | ||
| (DecoderImpl::Symphonia(source, PhantomData), sample) | ||
| } | ||
| DecoderImpl::None(unreachable, phantom) => { |
There was a problem hiding this comment.
Or DecoderImpl::None(_, _) => unreachable!()?
There was a problem hiding this comment.
Then the code after the match statement is dead code and causes a warning.
2 participants
I use
--no-default-features -F vorbis -F playbackwhich causes an unused code warning. While fixing this I found more warnings that you get when compiling without any decoder feature enabled. This PR fixes these warnings