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

Lint

Lint Skipped

Unit

Unit Tests Skipped

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 (22 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 20195

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 20195

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