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

fix build with Qt5.14 #5583

Merged
merged 2 commits into from
Jan 9, 2020
Merged

fix build with Qt5.14 #5583

merged 2 commits into from
Jan 9, 2020

Conversation

korli
Copy link
Contributor

@korli korli commented Jan 3, 2020

Resolves: https://musescore.org/en/node/299162

(short description of the changes and the motivation to make the changes)

Use "x" letter to fill the checkboxes below like [x]

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

Qt 5.14 introduces serialisation/deserialisation for enum classes,
this results in ambiguous templates between qdatastream.h and preferences.h.
we specialize everything to workaround this.
@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jan 3, 2020

You'd need to sign the CLA, else there's no chance for getting this PR merged, as far as I know.

This seems related to https://musescore.org/en/node/299162, right?

@t-bltg
Copy link

t-bltg commented Jan 3, 2020

I can confirm that this PR solves https://musescore.org/en/node/299162 on a linux OS.
Thanks

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jan 3, 2020

Interesting, QString::null is deprecated snce Qt 5.9 apparently, from qstring.h (from Qt 5.9.9 and 5.12.6):

#if QT_DEPRECATED_SINCE(5, 9)
    struct Null { };
    QT_DEPRECATED_X("use QString()")
    static const Null null;
    inline QString(const Null &): d(Data::sharedNull()) {}
    inline QString &operator=(const Null &) { *this = QString(); return *this; }
#endif

But why doesn't it lead to warnings using Qt 5.9 or 5.12? Anyway, that change seems to be OK

@t-bltg
Copy link

t-bltg commented Jan 3, 2020

@Jojo-Schmitz It seems that one has to define QT_DEPRECATED_WARNINGS in order to throw compiler warnings.
E.g. adding to CMakeLists.txt

add_compile_definitions(QT_DEPRECATED_WARNINGS)

However, QT_DEPRECATED_WARNINGS has no effect since Qt 5.13
Source: https://doc.qt.io/qt-5/qtglobal.html#QT_DEPRECATED_WARNINGS

@Jojo-Schmitz
Copy link
Contributor

Strange then how @korli found those warnings? Or did they get enabled again and possible even by default for Qt 5.14.0?

@korli
Copy link
Contributor Author

korli commented Jan 3, 2020

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jan 3, 2020

So QT_DEPRECATED_WARNINGS got changed to have no effect, but at the same time QT_NO_DEPRECATED_WARNINGS got introduced to surpress those? Confusing...

@korli
Copy link
Contributor Author

korli commented Jan 3, 2020

You'd need to sign the CLA, else there's no chance for getting this PR merged, as far as I know.

Done!

This seems related to https://musescore.org/en/node/299162, right?

It seems so.

@korli
Copy link
Contributor Author

korli commented Jan 3, 2020

So QT_DEPRECATED_WARNINGS got changed to have no effect, but at the same time QT_NO_DEPRECATED_WARNINGS got introduced to surpress those? Confusing...

I understand that deprecation warnings become default.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jan 3, 2020

You'd need to sign the CLA, else there's no chance for getting this PR merged, as far as I know.

Done!

Thanks!

This seems related to https://musescore.org/en/node/299162, right?

It seems so.

You could use that link in the PR description under "Resolves: (direct link to the issue)", even if it is not an issue but a forum article

@korli
Copy link
Contributor Author

korli commented Jan 7, 2020

You could use that link in the PR description under "Resolves: (direct link to the issue)", even if it is not an issue but a forum article

Thanks for the tip!

{
return out << static_cast<int>(val);
}

template<typename T, typename std::enable_if<std::is_enum<T>::value>::type* = nullptr>
inline QDataStream &operator>>(QDataStream &in, T &val)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did these templates prevent MuseScore from compiling? Could you please post the error message then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

So this seems to be because of these lines which were added in Qt 5.14:
https://github.com/qt/qtbase/blob/5.14.0/src/corelib/serialization/qdatastream.h#L387-L395

Actually Qt implementation of these operators seems to do the same but maybe it is indeed better to be explicit here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Qt implementation of these operators seems to do the same but actually differs: the build fails with it. This would need investigation but changes would be more involved.

@dmitrio95 dmitrio95 merged commit 8b9a81e into musescore:master Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants