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

SampleRate: Minor fix and improvement #4091

Merged
merged 2 commits into from
Jul 9, 2021
Merged

SampleRate: Minor fix and improvement #4091

merged 2 commits into from
Jul 9, 2021

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Jul 8, 2021

No description provided.

static SampleRate fromDouble(double value) {
const auto sampleRate = SampleRate(static_cast<value_t>(value));
// The sample rate should always be an integer value
// and this conversion is supposed to be lossless.
Copy link
Member

@Holzhaus Holzhaus Jul 8, 2021

Choose a reason for hiding this comment

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

good idea. This also catches cases where value is NaN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not tested. Probably yes, because there is no NaN integer.

}

constexpr double toDouble() {
constexpr double toDouble() const {
Copy link
Member

Choose a reason for hiding this comment

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

😨 oops

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@Holzhaus
Copy link
Member

Holzhaus commented Jul 8, 2021

Waiting for CI.

@Holzhaus Holzhaus merged commit 787b8aa into mixxxdj:main Jul 9, 2021
@uklotzde uklotzde deleted the samplerate branch July 9, 2021 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants