-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix compiler warnings (master) #6588
fix compiler warnings (master) #6588
Conversation
47dc1f1
to
ed7fb0b
Compare
as seen in MinGW and/or MSVC
and FALLTRHOUGH cleanup
and remove all conditions checking for Qt versions
ed7fb0b
to
e693c56
Compare
case QSysInfo::MV_10_13: | ||
os = "Macintosh; Mac OS 10.13"; | ||
break; | ||
case QSysInfo::MV_10_14: | ||
os = "Macintosh; Mac OS 10.14"; | ||
break; | ||
case QSysInfo::MV_10_15: | ||
os = "Macintosh; Mac OS 10.15"; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice try, but these constants do not exist, and so this fails to build on macOS. In fact, the existing QSysInfo
constants are deprecated. Better to rewrite the function like this:
#ifdef Q_OS_MAC
/**
* Only on Mac OS X
* Get the Operating system name and version.
* @return os The operating system name and version in a string.
*/
QString GAnalytics::Private::getSystemInfo()
{
auto osType = QOperatingSystemVersion::currentType();
auto version = QOperatingSystemVersion::current();
QString os;
switch (osType)
{
case QOperatingSystemVersion::MacOS:
os = QString("Macintosh; Mac OS ");
break;
case QOperatingSystemVersion::IOS:
os = QString("iPhone; iOS ");
break;
default:
break;
}
os += QString("%1.%2").arg(version.majorVersion()).arg(version.minorVersion());
return os;
}
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm pretty sure I looked them up in the Qt 5.15.1 header files?
And the CI tests all passed too!
Will need to look again...
But feel free to PR your proposal ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, you're right, those don't seem to exist. But your proposal doesn't build either, there seems to be no such thing as a class QOperatingSystemVersion
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It exists. You just need to #include <QOperatingSystemVersion>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, there are no CI tests to make sure that MuseScore builds on macOS. Otherwise, it would have failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, #include <QOperatingSystemVersion>
helps there... (posts crossed)
Of course there are CI tests for Mac, see https://github.com/musescore/MuseScore/runs/1208486289 and https://github.com/musescore/MuseScore/runs/1208486341 for this very PR.
The problem is those don't report the build fails as an error! @igorkorsukov ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check #6647
as seen in MinGW and/or MSVC