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

Build: Enable warning -Woverloaded-virtual #3046

Merged
merged 5 commits into from
Aug 29, 2020
Merged

Build: Enable warning -Woverloaded-virtual #3046

merged 5 commits into from
Aug 29, 2020

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Aug 25, 2020

This option helps to detect unintended overloading of non-virtual methods from base classes. Could have detected the obsolete slots fixed in #3044.

As a prerequisite, some fixes in the controller subsystem were required. The backport to 2.3 would require some effort due to differences in the class hierarchy. I neither have the time nor motivation to sort this out.

@uklotzde
Copy link
Contributor Author

@Holzhaus I didn't find a solution for suppressing the following warning during the build:

[264/681] Building C object CMakeFiles/mixxx-lib.dir/src/musicbrainz/crc.c.o
cc1: warning: command-line option ‘-Woverloaded-virtual’ is valid for C++/ObjC++ but not for C

cc1: warning: command-line option ‘-Woverloaded-virtual’ is valid for C++/ObjC++ but not for C
@uklotzde
Copy link
Contributor Author

@Holzhaus I didn't find a solution for suppressing the following warning during the build:

[264/681] Building C object CMakeFiles/mixxx-lib.dir/src/musicbrainz/crc.c.o
cc1: warning: command-line option ‘-Woverloaded-virtual’ is valid for C++/ObjC++ but not for C

"Fixed" by renaming the file 🙈

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, works fine on my machine. Waiting for CI.

@uklotzde
Copy link
Contributor Author

@Holzhaus Wait a moment. The renamed Q_INVOCABLE in HidControllerJSProxy could cause issues. I'll check if this was really necessary.

@uklotzde
Copy link
Contributor Author

HidControllerJSProxy

Reverted. Locally builds without errors.

@uklotzde
Copy link
Contributor Author

OSX/clang/SCons spoils all Travis CI builds. We need to get rid of it.

@uklotzde uklotzde added this to the 2.4.0 milestone Aug 27, 2020
@uklotzde
Copy link
Contributor Author

Ping

@uklotzde
Copy link
Contributor Author

Pingping

@Holzhaus Holzhaus merged commit b534d76 into mixxxdj:master Aug 29, 2020
@uklotzde uklotzde deleted the Woverloaded-virtual branch August 29, 2020 15:56
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