Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/decoder/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,10 @@ impl<R: Read + Seek + Send + Sync + 'static> DecoderBuilder<R> {
}

#[cfg(not(feature = "symphonia"))]
Err(DecoderError::UnrecognizedFormat)
{
let _ = data;
Err(DecoderError::UnrecognizedFormat)
Copy link
Member

Choose a reason for hiding this comment

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

Tagging #[allow(unused_variables)] may convey intent more clearly.

Copy link
Author

Choose a reason for hiding this comment

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

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.

}
}

/// Creates a new decoder with previously configured settings.
Expand Down
18 changes: 15 additions & 3 deletions src/decoder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ pub struct Decoder<R: Read + Seek>(DecoderImpl<R>);
pub struct LoopedDecoder<R: Read + Seek> {
/// The underlying decoder implementation.
inner: Option<DecoderImpl<R>>,

/// Configuration settings for the decoder.
settings: Settings,
}
Expand Down Expand Up @@ -249,7 +250,10 @@ impl<R: Read + Seek> DecoderImpl<R> {
DecoderImpl::Mp3(source) => source.try_seek(pos),
#[cfg(feature = "symphonia")]
DecoderImpl::Symphonia(source, PhantomData) => source.try_seek(pos),
DecoderImpl::None(_, _) => unreachable!(),
DecoderImpl::None(_, _) => {
let _ = pos;
Copy link
Member

Choose a reason for hiding this comment

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

Trick here is to make the argument _pos in the function signature.

Copy link
Author

Choose a reason for hiding this comment

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

Starting a variable with underscore is used for unused variables. pos is not generally unused.

Copy link
Member

Choose a reason for hiding this comment

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

You can use it as _pos too: source.try_seek(_pos).

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but naming it _pos communicates the intent of it being unused - which it is not generally.

unreachable!()
}
}
}
}
Expand Down Expand Up @@ -622,6 +626,7 @@ where

// Take ownership of the decoder to reset it
let decoder = self.inner.take()?;

let (new_decoder, sample) = match decoder {
#[cfg(all(feature = "hound", not(feature = "symphonia-wav")))]
DecoderImpl::Wav(source) => {
Expand Down Expand Up @@ -667,9 +672,16 @@ where
let sample = source.next();
(DecoderImpl::Symphonia(source, PhantomData), sample)
}
DecoderImpl::None(unreachable, phantom) => {
Copy link
Member

Choose a reason for hiding this comment

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

Or DecoderImpl::None(_, _) => unreachable!()?

Copy link
Author

Choose a reason for hiding this comment

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

Then the code after the match statement is dead code and causes a warning.

let _ = self.settings;
(DecoderImpl::None(unreachable, phantom), None)
}
};
self.inner = Some(new_decoder);
sample

{
self.inner = Some(new_decoder);
sample
}
} else {
None
}
Expand Down
4 changes: 2 additions & 2 deletions src/source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::sync::Arc;
use crate::{
buffer::SamplesBuffer,
common::{assert_error_traits, ChannelCount, SampleRate},
math, BitDepth, Float, Sample,
math, Float, Sample,
};

use dasp_sample::FromSample;
Expand Down Expand Up @@ -231,7 +231,7 @@ pub trait Source: Iterator<Item = Sample> {
#[cfg(feature = "dither")]
#[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>
Copy link
Member

Choose a reason for hiding this comment

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

What is your rationale here?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

👍

where
Self: Sized,
{
Expand Down