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

Fix windows Build: activate QT initializer lists #1019

Merged
merged 4 commits into from
Sep 28, 2016
Merged

Fix windows Build: activate QT initializer lists #1019

merged 4 commits into from
Sep 28, 2016

Conversation

sblaisot
Copy link
Member

This PR fixes windows build lp:1627826

For the record:
even if MSVC 2013 compiler supports initializer lists, QT 4.8 explicitly disables initializer lists with MSVC due to old MSVC 2012 bug.
http://code.qt.io/cgit/qt/qt.git/tree/src/corelib/global/qglobal.h#n910

@Be-ing
Copy link
Contributor

Be-ing commented Sep 28, 2016

Does key sorting actually work, or does it just compile a broken program?

@sblaisot
Copy link
Member Author

does unit test let me know if key sorting work ?

@sblaisot
Copy link
Member Author

currently, it compiles a program that runs, I can click the "key" column header to sort, I obtain track list sorted by C -> Am -> G -> Em -> D -> Bm -> A -> F#m -> E -> C#m -> B -> G#m -> F#/Gb -> ...

@Be-ing
Copy link
Contributor

Be-ing commented Sep 28, 2016

No, there isn't an automated test for it.

currently, it compiles a program that runs, I can click the "key" column header to sort, I obtain track list sorted by C -> Am -> G -> Em -> D -> Bm -> A -> F#m -> E -> C#m -> B -> G#m -> F#/Gb -> ...

That's working :)

Be-ing
Be-ing reviewed Sep 28, 2016
View reviewed changes
src/track/keyutils.cpp
@@ -1,3 +1,11 @@
// Horrible Hack to workaround QT 4.8 bug with MSVC compiler. lp:1627826
// QT 4.8 explicitely disable Initializer list, but initializer list is
// supported with MSVC 2012 SP2 +
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean MSVC 2013 SP2?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I mean 2012 SP2. Initialiser lists are broken before 2012 SP2. They have always been OK with 2013, but disabled by QT4 code.

Copy link
Member Author

@sblaisot sblaisot Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

you were right. I meant VC12, which is VS2013. updated.

Pegasus-RPG
Pegasus-RPG reviewed Sep 28, 2016
View reviewed changes
src/track/keyutils.cpp
// supported with MSVC 2012 SP2 +
// http://code.qt.io/cgit/qt/qt.git/tree/src/corelib/global/qglobal.h#n910
// TODO(XXX): Remove after QT4 deprecation, unneeded with QT5
#ifdef __WINDOWS__
Copy link
Member

@Pegasus-RPG Pegasus-RPG Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with Zaren, change this to #if _MSC_VER >= 1800 // VC 12 (Or #if _MSC_FULL_VER >= 180030324 if you prefer.)

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

@czchmate
Copy link

"#if _MSC_FULL_VER >= 180030324" does not work on my copy of Visual Studio 2013. My copy of Visual Studio 2013 is up to date.

You need to use "_MSC_VER >= 1800" to allow Visual Studio 2013 and above.

@sblaisot
Copy link
Member Author

@czchmate strange, this means you can not use initializer lists event qith QT5. This piece of code is exactly taken from QT5's sources: https://gitorious.org/qt/qtbase?p=qt:qtbase.git;a=blob;f=src/corelib/global/qcompilerdetection.h;h=f8a8a436bec0d607a0bb58bf6bd455ac79afb0f7;hb=HEAD#l874

@sblaisot
Copy link
Member Author

@czchmate what is the version reported by cl /?

@daschuer
Copy link
Member

Thank you for taking care :-)

I am just wonder why you limit the hack to a specific VC version?
If the compiler is to old, build will fail anyway and it does not matter if the hack is involved or not.

Is it merge-able?

@czchmate
Copy link

I'm using compiler 18.00.21005.1 with QT 4.8.6. The Mixxx Windows build instructions say to use 4.8.6, https://www.mixxx.org/wiki/doku.php/compiling_on_windows.

@czchmate
Copy link

Maybe I was unclear.

I am using compiler 18.00.21005.1 (Visual Studio Express 2013) with QT 4.8.6. Using the following code allows the program to compile:

#if _MSC_VER >= 1800

Compilation fails using the other code:

"#if _MSC_FULL_VER >= 180030324

The version is incorrect.

All versions of Visual Studio 2013 or higher (_MSC_VER >= 1800) can use initializer lists. This is shown in Microsoft's documentation: https://msdn.microsoft.com/en-us/en-en/library/hh567368.aspx.

There is no reason to go to use _MSC_FULL_VER when _MSC_VER will suffice.

@sblaisot
Copy link
Member Author

sblaisot commented Sep 28, 2016 •
edited
Loading

@daschuer

  • yes; ready to merge
  • I first added an ifdef to protect this hack on windows platform only. But @Pegasus-RPG and @czchmate wanted the #ifdef _MSC_VER. they're two, you are one only, and I don't have an opinion on this ;)
  • The problem is not when the compiler doesn't implement initializer lists, it's when the compiler pretends to support it but it's buggy. If I understand well what I've read about that, MSVS 2013 SP2 is ok, but MSVS 2013 is buggy.

@sblaisot
Copy link
Member Author

@czchmate
from https://bugreports.qt.io/browse/QTBUG-39142
"/The problem is that while VS 2013 in theory supports initializer lists, it has several bugs that event prevent Qt unit tests from compiling.
These are fixed in service pack 2, so the support was bumped to that version./"

@sblaisot
Copy link
Member Author

sblaisot commented Sep 28, 2016 •
edited
Loading

additionally, not all flavor of initializer lists are implemented in MSVS: https://msdn.microsoft.com/en-us/library/dn793970.aspx
with bad code generation until VS2013SP3 and compilation failure starting from MSVS2013SP3

@daschuer
Copy link
Member

Ok, It looks like the issue is fixed for now. So lets merge it.
Thank you!

@daschuer daschuer merged commit 20e5f2c into mixxxdj:master Sep 28, 2016
@Pegasus-RPG
Copy link
Member

@czchmate Yes, we need to update the Wiki to say 4.8.7. Sorry about that.

@Be-ing
Copy link
Contributor

Be-ing commented Sep 28, 2016

Thanks for taking care of this!

@czchmate
Copy link

Yes, thanks!

Your speed at fixing the bugs I have reported has been tremendous. Top notch development team here!

@sblaisot sblaisot deleted the win_compile_fix branch September 29, 2016 06:13
@mixxxbot mixxxbot mentioned this pull request Aug 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Be-ing Be-ing Be-ing left review comments

@Pegasus-RPG Pegasus-RPG Pegasus-RPG left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sblaisot @Be-ing @czchmate @daschuer @Pegasus-RPG