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

MSVC warning fix #4867

Merged
merged 13 commits into from
Jul 31, 2022
Merged

MSVC warning fix #4867

merged 13 commits into from
Jul 31, 2022

Conversation

daschuer
Copy link
Member

This fixes a couple of warnings that are generated by MSVC. See commit messages.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks. How much effort would it be to contribute the libshout related changed upstream?

src/controllers/controller.cpp Outdated Show resolved Hide resolved
@@ -458,7 +458,7 @@ void SeratoBeatGrid::setBeats(BeatsPointer pBeats,
}
}

nonTerminalMarkers.reserve(markers.size());
nonTerminalMarkers.reserve(static_cast<int>(markers.size()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
nonTerminalMarkers.reserve(static_cast<int>(markers.size()));
nonTerminalMarkers.reserve(static_cast<qsizetype>(markers.size()));

To remain warning free with Qt6.

Please be careful in general when casting size-related types to int.

Copy link
Member Author

Choose a reason for hiding this comment

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

nonTerminalMarkers is a QList in Qt5 it has reserve(int alloc)
A qsizetype cast is a std::ptrdiff_t in Qt5 and Qt6 so the proposed change will become a lossy operation in Qt5.
So for now the current solution works warning free.
We may consider to introduce a type that is qsizetype in Qt 6 and int in Qt 5 to make it correct. But IMHO there is no technical need and that will be surprising cast for all new contributors.

Copy link
Member

Choose a reason for hiding this comment

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

Mhmm yeah, thats unfortunate. Those warnings will come back for Qt6 though since there reserve takes a qsizetype (I was hoping it was just int on Qt5). Introducing ifdefs everywhere is also bad. I guess its better to have a widening conversion than narrowing... so I guess we can keep int.

@daschuer
Copy link
Member Author

Thanks. How much effort would it be to contribute the libshout related changed upstream?

We need to do that on the main branch of libshout, since we are using an old version and the idjc fork, it is not directly beneficial. But maybe I feel lucky to do it ...

@Swiftb0y
Copy link
Member

Mhmm ok, no worries. Just figured I'd ask.

@daschuer
Copy link
Member Author

Done.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@daschuer
Copy link
Member Author

@Swiftb0y Can this be merged?

@Swiftb0y
Copy link
Member

Yep, wanted to give others a chance to comment too.

@Swiftb0y Swiftb0y merged commit efa0d27 into mixxxdj:main Jul 31, 2022
@daschuer daschuer deleted the msvc_warnings branch November 16, 2022 08:34
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