| cgilles | |
| kfunk |
| Frameworks | |
| Build System |
A recent commit disabled support for named (logical) operators (and, not, bitand, &c) when using GCC, Clang or ICC.
This patch adds a function to (re)enable them in a cross-platform fashion in projects where they are required, the same way C++ exceptions are handled.
The MSVC-specific part will (hopefully) activate named operator support in MSVC 2017 and later, without side-effects (again, hopefully). For earlier versions a warning is printed as well as when using other compilers.
Tested on Mac and Linux (with digiKam 5.5.0).
The same warning is printed when using an unsupported MSVC version or a compiler supporting -fnamed-operators, following Thiago's wording. It might be better to warn against the purpose instead of against using the macro, esp. when an unsupported MSVC is being used.
| Lint Skipped |
| Unit Tests Skipped |
LGTM in general. I'll hope to find some time to test on Window, wouldn't want this to be merged before that.
| kde-modules/KDECompilerSettings.cmake | ||
|---|---|---|
| 226 | MSVC_VERSION GREATER 1900 is sufficient. No need for ${...} | |
I'll hope to find some time to test on Window, wouldn't want this to be merged before that.
Should we read "not to be merged before *you* have tested it"? I have nothing against that, esp. if you also find the exact value to use for the MSC_VERSION test ;)
| kde-modules/KDECompilerSettings.cmake | ||
|---|---|---|
| 226 | Will fix that before committing anything. | |
Can't go in as-is, as it breaks compilation on MSVC 2015.
But interesting to see that /Za is actually problematic. Which proves my point: Just don't use named operators in code which claims to be cross-platform.
| kde-modules/KDECompilerSettings.cmake | ||
|---|---|---|
| 230 | Doesn't work, breaks compilation inside WinSDK 10: FAILED: src/CMakeFiles/KF5Parts.dir/readwritepart.cpp.obj C:\PROGRA~2\MICROS~2.0\VC\bin\amd64\cl.exe /nologo /TP -DKCOREADDONS_LIB -DKF5Parts_EXPORTS -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_CAST_TO_ASCII -DQT_NO_DEBUG -DQT_NO_SIG NALS_SLOTS_KEYWORDS -DQT_NO_URL_CAST_FROM_STRING -DQT_USE_FAST_OPERATOR_PLUS -DQT_USE_QSTRINGBUILDER -DQT_WIDGETS_LIB -DQT_XML_LIB -DTRANSLATION_DOMAIN=\"kparts5\" -DUNICODE -DWIN32_LEAN_AND_MEAN -DW INVER=0x0600 -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D_USE_MATH_DEFINES -D_WIN32_IE=0x0600 -D_WIN32_WINNT=0x0600 -Isrc - IZ:\kderoot\download\git\kparts\src -I. -IZ:\kderoot\include\KF5\KIOWidgets -IZ:\kderoot\include\KF5 -IZ:\kderoot\include\KF5\KIOCore -IZ:\kderoot\include\KF5\KCoreAddons -IZ:\kderoot\include\qt5 -IZ :\kderoot\include\qt5\QtCore -IZ:\kderoot\.\mkspecs\win32-msvc2015 -IZ:\kderoot\include\KF5\KService -IZ:\kderoot\include\KF5\KConfigCore -IZ:\kderoot\include\KF5\KJobWidgets -IZ:\kderoot\include\qt5 \QtWidgets -IZ:\kderoot\include\qt5\QtGui -IZ:\kderoot\include\qt5\QtNetwork -IZ:\kderoot\include\KF5\KCompletion -IZ:\kderoot\include\KF5\KWidgetsAddons -IZ:\kderoot\include\KF5\KXmlGui -IZ:\kderoot \include\qt5\QtDBus -IZ:\kderoot\include\qt5\QtXml -IZ:\kderoot\include\KF5\KConfigWidgets -IZ:\kderoot\include\KF5\KCodecs -IZ:\kderoot\include\KF5\KConfigGui -IZ:\kderoot\include\KF5\KAuth -IZ:\kde root\include\KF5\KTextWidgets -IZ:\kderoot\include\KF5\SonnetUi -IZ:\kderoot\include\KF5\KI18n -IZ:\kderoot\include\KF5\KIconThemes /DWIN32 /D_WINDOWS /W3 /GR /EHsc /wd4250 /wd4251 /wd4396 /wd4661 /Z a /MD /Zi /O2 /Ob1 /DNDEBUG /showIncludes /Fosrc\CMakeFiles\KF5Parts.dir\readwritepart.cpp.obj /Fdsrc\CMakeFiles\KF5Parts.dir\ /FS -c Z:\kderoot\download\git\kparts\src\readwritepart.cpp C:\Program Files (x86)\Windows Kits\10\include\10.0.14393.0\um\winnt.h(12128): error C2467: illegal declaration of anonymous 'struct' Known issue: https://stackoverflow.com/questions/5489326/za-compiler-directive-does-not-compile-system-headers-in-vs2010 Apparently Microsoft doesn't even test their own headers against compiling with /Za | |
| 231 | Typo: else() then this branch is actually executed. | |
| 239 | Doesn't work on MSVC 2015 Update 3: cl : Command line warning D9002 : ignoring unknown option '/permissive-' My Version: Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64 | |
So, how do we proceed?
When I asked other devs what compiler they used to build their MSWin bundles they all (both) said they can't use MSVC anyway because it's too buggy.
If a compiler is problematic in general it may not be worth it trying to account for it in a macro, trying to coerce it into doing something it cannot handle. I don't think that's a reason not to implement the macro at all though. Even if we cannot figure out from what version onwards the /fpermissive- option works it could still be useful for cross-platform code that really needs named operator support to let the macro print a warning (or raise an error) when it is built with MSVC.
| kde-modules/KDECompilerSettings.cmake | ||
|---|---|---|
| 239 | Have you been (or are you) able to check from what version the option is accepted? | |
That option only exists in MSVC 2017. Qt 5.9.1 will use it for its own build. So far, so good.
But that has nothing to do with the named keywords. And the problem is not the compiler, it's Microsoft's Windows SDK headers.
That option only exists in MSVC 2017.
Should we have deduce that from the docs I cite in the CMake comments? I'm willing to believe that I misread as far as support for named operators is concerned, but they do mention 2015 Update 3 specifically as the appearance of /permissive-.
But that has nothing to do with the named keywords. And the problem is not the compiler, it's Microsoft's Windows SDK headers.
In what sense? Do they use reserved keywords or is the named operator support implemented in header files with macros or who knows what?
And just to be clear, there is thus no reliable way to obtain support for named operators in MSVC, not even with v. 2017 (and foreseeable newer versions)?
Documentation to the rescue:
But that has nothing to do with the named keywords. And the problem is not the compiler, it's Microsoft's Windows SDK headers.
In what sense? Do they use reserved keywords or is the named operator support implemented in header files with macros or who knows what?
Yes, and more. For example, MSVC 2013 had -Zc:strictStrings, but we couldn't turn it on because the headers would then fail to compile. This improved in MSVC 2015, so we turned it on.
And just to be clear, there is thus no reliable way to obtain support for named operators in MSVC, not even with v. 2017 (and foreseeable newer versions)?
Why are we spending time in named operators? It's much easier to fix the source code not to use them and then the problem goes away.
This is about adding a convenience macro for projects that have dependencies using named operators, like certain Boost modules.
| kde-modules/KDECompilerSettings.cmake | ||
|---|---|---|
| 216–218 | Shouldn't this be checking for CMAKE_CXX_COMPILER_ID? | |
Fix the source code.
If certain third-party libraries can't compile under MSVC 2015 (or even 2017!) with default compiler options, they don't deserve to be used. Blacklist them.
That may work for Qt but is not an acceptable attitude for ECM, IMHO - and MSVC is problematic enough to build KF5 code for other reasons not to jump through hoops for it. You cannot expect FOSS projects to avoid Boost because it uses less common feature of the standard, big ole bad M$ cannot be bothered to write a proper compiler. I'd be all for blacklisting such compilers, if anything had to be blacklisted.
The projects I'm aware of that use "incriminated" boost code do provide MSWin builds but using a properly standards-compliant compiler.
My proposal is thus to drop MSVC support in the new macro, printing a warning should be enough.
This is in a kde.org codereview of a macro with KDE in the name. I think it's acceptable. Don't waste time with code that doesn't try to be cross-platform. Just let it fail to compile.
And submit bug reports to those libraries. It's very easy for them to fix.
The projects I'm aware of that use "incriminated" boost code do provide MSWin builds but using a properly standards-compliant compiler.
My proposal is thus to drop MSVC support in the new macro, printing a warning should be enough.
That's a fine solution. If you're going to print a warning and you're serious about cross-platformness support, I suggest printing the warning for ALL compilers: "Warning: using C++ operator names is not compatible with MSVC prior to version 2017 (19.1). You should reconsider using this macro if your code is meant to be cross-platform".
KDE is FOSS not bound to Microsoft by any corporate buy-in or whatever, right?
If there's a bug to report it's the lack of standard compliance in MSVC - how have MS reacted to such reports?
What non-sense is this? Please stay on topic. There's a benefit we make sure KDE software is compiling under MSVC given its popularity on Windows.
If there's a bug to report it's the lack of standard compliance in MSVC - how have MS reacted to such reports?
... and you're going to have them fix older MSVC versions to support your use-case? This discussion is such a waste of time really. Please face the reality and show some pragmatism: Named operators do not work on MSVC prior to MSVC 2017, period.
+1 on Thiago's solution, for the good. Let's just print a warning for MSVC prior to version 2017:
That's a fine solution. If you're going to print a warning and you're serious about cross-platformness support, I suggest printing the warning for ALL compilers: "Warning: using C++ operator names is not compatible with MSVC prior to version 2017 (19.1). You should reconsider using this macro if your code is meant to be cross-platform".
Updated as requested.
GIven the controversy I thought it might be useful to add at least a target-specific enabler macro (which may need some polishing or simplification - using generator expressions may not be required here? Do we also want the per-file version?
Not that I want to have the last word, but it's not me who brought up fixing code and filing bug reports. And contrary to FOSS projects, MS *do* have the resources and the means to push patches to all currently supported versions of their product.
That's because of the MSVC remarks you made inline?
I don't have a way to test with MSVC, so I could do with some suggestions how to resolve the flagged issues.