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.

Sign up for GitHub

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

Jump to bottom

CMake: Enable -Wfloat-conversion warnings #3129

Merged
merged 1 commit into from
Oct 18, 2020
Merged

CMake: Enable -Wfloat-conversion warnings #3129

merged 1 commit into from
Oct 18, 2020

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Sep 25, 2020 •
edited by uklotzde
Loading

@Holzhaus Holzhaus added this to the 2.3.0 milestone Sep 25, 2020
@Holzhaus
Copy link
Member Author

Ping. Please have a look at the unmerged PRs linked above so that we can merge this. I hope this will avoid introducing new unintended-precision-loss bugs like #3119.

@Holzhaus Holzhaus force-pushed the cmake-float-conversion-warnings branch from 6187491 to 56b9bc0 Compare October 16, 2020 08:28
@Holzhaus
Copy link
Member Author

Holzhaus commented Oct 16, 2020 •
edited
Loading

I rebased on latest 2.3, we still have a few warnings. I'll file a new PR to fix them.

EDIT: Just two commits, added them here.

@Holzhaus Holzhaus force-pushed the cmake-float-conversion-warnings branch from 56b9bc0 to 4a8461f Compare October 16, 2020 08:47
@Holzhaus Holzhaus requested a review from daschuer October 16, 2020 08:47
daschuer
daschuer requested changes Oct 16, 2020
View reviewed changes
src/waveform/renderers/waveformrendererpreroll.cpp Outdated
int currentPosition = m_waveformRenderer->getPlayPosVSample();
int totalSamples = m_waveformRenderer->getTotalVSample();
int currentPosition = static_cast<int>(m_waveformRenderer->getPlayPosVSample());
int totalSamples = static_cast<int>(m_waveformRenderer->getTotalVSample());
Copy link
Member

Choose a reason for hiding this comment

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

Ups sorry. you have hit a rats nest.
getPlayPosVSample() should directly return int, because it is the already rounded VSample.
However I think we have some rounding issues here, because m_visualSamplePerPixel is double and can be changed in fine steps since we have removed the stepping from waveform zooms.
I think we need to review all position calculations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, see #3176.

@Holzhaus Holzhaus force-pushed the cmake-float-conversion-warnings branch from 4a8461f to 34d0c9f Compare October 16, 2020 11:32
@Holzhaus Holzhaus mentioned this pull request Oct 16, 2020
Merged
@uklotzde
Copy link
Contributor

Ready after rebasing?

@Holzhaus Holzhaus force-pushed the cmake-float-conversion-warnings branch from 34d0c9f to 2fa3102 Compare October 17, 2020 20:29
@Holzhaus
Copy link
Member Author

Ready after rebasing?

Mixxx is now warning-free on my machine. Let's check if the CI agrees.

@daschuer
Copy link
Member

We have now a failing test on MSVC:

The following tests FAILED:
256 - EngineSyncTest.HalfDoubleThenPlay (Failed)
Errors while running CTest

@Holzhaus
Copy link
Member Author

We have now a failing test on MSVC:

The following tests FAILED:
256 - EngineSyncTest.HalfDoubleThenPlay (Failed)
Errors while running CTest

This is unrelated to this PR. It only changes the compiler warnings, no actual code.

But I can look into it, although it will be hard to debug with remote debugging via CI.

uklotzde
uklotzde approved these changes Oct 18, 2020
View reviewed changes
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

LGTM

@uklotzde
Copy link
Contributor

I also noticed the recent test failures on AppVeyor for Windows that must be caused by some other changes. Unfortunately, we are not able to distinguish those actual failures from spurious failures as long as the tests are not reliable.

@uklotzde
Copy link
Contributor

@daschuer Merge?

daschuer
daschuer approved these changes Oct 18, 2020
View reviewed changes
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Ok, right, let's merge.

@daschuer daschuer merged commit 10e707a into mixxxdj:2.3 Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschuer daschuer daschuer approved these changes

@uklotzde uklotzde uklotzde approved these changes

Assignees
No one assigned
Labels
build code quality
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Holzhaus @uklotzde @daschuer