Details

Reviewers

drosca
kfunk

Commits

R875:ebd8dd051e18: Windows: Fix compile issues after build system changed to cmake.

Summary

After moving to cmake there are some compile issues on Windows. This patch fixes them, but as I am not very familiar with cmake, maybe adding some options to cmake configuration is the right solution?

Diff Detail

Repository

R875 Falkon

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

ralavizadeh created this revision.Sep 30 2017, 10:02 PM

kfunk requested changes to this revision.Oct 1 2017, 10:20 AM

kfunk added a subscriber: kfunk.

kfunk added inline comments.

src/lib/other/registerqappassociation.cpp
24	`0x0602` means Windows 8 and above, right? Does that mean Falkon is not buildable on a Windows 7 system? Should be worthwhile getting Falkon to build on a Win7 system. Here's the excerpt from MSDN for the meaning of `_WIN32_WINNT`: // // _WIN32_WINNT version constants // #define _WIN32_WINNT_NT4 0x0400 // Windows NT 4.0 #define _WIN32_WINNT_WIN2K 0x0500 // Windows 2000 #define _WIN32_WINNT_WINXP 0x0501 // Windows XP #define _WIN32_WINNT_WS03 0x0502 // Windows Server 2003 #define _WIN32_WINNT_WIN6 0x0600 // Windows Vista #define _WIN32_WINNT_VISTA 0x0600 // Windows Vista #define _WIN32_WINNT_WS08 0x0600 // Windows Server 2008 #define _WIN32_WINNT_LONGHORN 0x0600 // Windows Vista #define _WIN32_WINNT_WIN7 0x0601 // Windows 7 #define _WIN32_WINNT_WIN8 0x0602 // Windows 8 #define _WIN32_WINNT_WINBLUE 0x0603 // Windows 8.1 #define _WIN32_WINNT_WINTHRESHOLD 0x0A00 // Windows 10 #define _WIN32_WINNT_WIN10 0x0A00 // Windows 10

src/lib/other/registerqappassociation.cpp

0x0602 means Windows 8 and above, right?

Does that mean Falkon is not buildable on a Windows 7 system? Should be worthwhile getting Falkon to build on a Win7 system.

Here's the excerpt from MSDN for the meaning of _WIN32_WINNT:

//  
// _WIN32_WINNT version constants  
//  
#define _WIN32_WINNT_NT4                    0x0400 // Windows NT 4.0  
#define _WIN32_WINNT_WIN2K                  0x0500 // Windows 2000  
#define _WIN32_WINNT_WINXP                  0x0501 // Windows XP  
#define _WIN32_WINNT_WS03                   0x0502 // Windows Server 2003  
#define _WIN32_WINNT_WIN6                   0x0600 // Windows Vista  
#define _WIN32_WINNT_VISTA                  0x0600 // Windows Vista  
#define _WIN32_WINNT_WS08                   0x0600 // Windows Server 2008  
#define _WIN32_WINNT_LONGHORN               0x0600 // Windows Vista  
#define _WIN32_WINNT_WIN7                   0x0601 // Windows 7  
#define _WIN32_WINNT_WIN8                   0x0602 // Windows 8  
#define _WIN32_WINNT_WINBLUE                0x0603 // Windows 8.1  
#define _WIN32_WINNT_WINTHRESHOLD           0x0A00 // Windows 10  
#define _WIN32_WINNT_WIN10                  0x0A00 // Windows 10

This revision now requires changes to proceed.Oct 1 2017, 10:20 AM

drosca added inline comments.Oct 1 2017, 10:41 AM

src/lib/app/datapaths.h
45	Why is an empty destructor needed?

drosca added inline comments.Oct 1 2017, 10:43 AM

src/lib/other/registerqappassociation.cpp
24	Isn't it more like it needs newer msvc than it needs to be built on Win8+?

ralavizadeh added inline comments.Oct 1 2017, 4:41 PM

src/lib/app/datapaths.h
45	The issue was with forward declaring QTemporaryDir. Qt documentation: Classes that are forward declared can be used within QScopedPointer, as long as the destructor of the forward declared class is available whenever a QScopedPointer needs to clean up. Concretely, this means that all classes containing a QScopedPointer that points to a forward declared class must have non-inline constructors, destructors and assignment operators. So I think MSVC compiler create inline destructor.
src/lib/other/registerqappassociation.cpp
24	0x0602 means Windows 8 and above, right? Yeah, you are right. The method showNativeDefaultAppSettingsUi() needs Win8+, will fix it.
24	Isn't it more like it needs newer msvc than it needs to be built on Win8+? I'm using MSVC2015. And the code was compilable when using qmake. I think qmake add Windows headers and define `_WIN32_WINNT` magically :D

Well, cmake set _WIN32_WINNT to 0x0600 (on my Windows 10 installation). This update adds a macro to cmake configuration to detect Windows version and defines _WIN32_WINNT.

Now it should be compilable on Vista.

Well, my last update removed other diff.

drosca accepted this revision.Oct 2 2017, 7:12 AM

drosca added inline comments.

CMakeLists.txt
84	If this is not needed, please remove this TODO note.

Closed by commit R875:ebd8dd051e18: Windows: Fix compile issues after build system changed to cmake. (authored by ralavizadeh). · Explain WhyOct 3 2017, 7:44 AM

This revision was automatically updated to reflect the committed changes.

			Path	Packages
M			CMakeLists.txt (23 lines)
M			src/lib/app/datapaths.h (1 line)
M			src/lib/app/datapaths.cpp (4 lines)
M			src/lib/downloads/downloaditem.cpp (1 line)
M			src/lib/other/registerqappassociation.cpp (77 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	20171			Sep 30 2017, 9:49 PM	★	★
Diff 2	20193			Oct 1 2017, 4:43 PM	★	★
Diff 3	20194			Oct 1 2017, 4:48 PM	★	★
Diff 4	20195			Oct 1 2017, 4:53 PM	★	★
Diff 5	20278	75bd63f	R875:ebd8dd051e18843c07f3f08c364e4db840cd8f5e	Oct 3 2017, 7:33 AM	★	★

[PATCH] Windows: Fix compile after build system changed to cmake.
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 20278

CMakeLists.txt

src/lib/app/datapaths.h

src/lib/app/datapaths.cpp

src/lib/downloads/downloaditem.cpp

src/lib/other/registerqappassociation.cpp

[PATCH] Windows: Fix compile after build system changed to cmake.ClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 20278

CMakeLists.txt

src/lib/app/datapaths.h

src/lib/app/datapaths.cpp

src/lib/downloads/downloaditem.cpp

src/lib/other/registerqappassociation.cpp

[PATCH] Windows: Fix compile after build system changed to cmake.
ClosedPublic
Actions

Revision Contents
Changeset List