Fixed windows.h can cause min/max conflicts
Changes PlannedPublic

Authored by anikolaev on Dec 21 2017, 11:27 AM.

Details

Reviewers
None
Group Reviewers
Windows

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
anikolaev requested review of this revision.Dec 21 2017, 11:27 AM
anikolaev created this revision.
anikolaev updated this revision to Diff 24261.Dec 21 2017, 6:14 PM
anikolaev updated this revision to Diff 24262.Dec 21 2017, 6:20 PM
anikolaev updated this revision to Diff 24264.Dec 21 2017, 6:24 PM
anikolaev set the repository for this revision to R486 QCA Library.
anikolaev removed 1 blocking reviewer(s): iromanov.
anikolaev edited reviewers, added: sitter; removed: iromanov.May 15 2018, 9:08 AM
sitter requested changes to this revision.Jun 19 2018, 9:56 AM
sitter added inline comments.
CMakeLists.txt
122

This seems overly complicated. You could simply use if(WIN32) add_definitions... from what I can tell setting it even when minmax isn't a macro has no downside and is in fact what we do in all other code maintained by KDE from what I can see.

crypto.prf.cmake
8

Doesn't this set it for consuming code? If so I don't think that is right, should only be set in cmakelists.txt. There isn't a single library where we force nominmax for consumers, even when we set it for ourselves in e.g. a cpp.

This revision now requires changes to proceed.Jun 19 2018, 9:56 AM
anikolaev requested review of this revision.Nov 6 2018, 10:12 AM
anikolaev marked 2 inline comments as done.

Hey, I've been waiting almost a year, what's the problem with my patch, why isn't it accepted? As for NOMINMAX, as you need this you must set this. Or make this "don't expose windows.h in qpipe.h, find another way to get HANDLE" (from TODO file)

CMakeLists.txt
122

For example MinGW has its own windows.h. So, probably, NOMINMAX is not needed for MinGW.
Also, I believe that someday min max macro will be deleted.

crypto.prf.cmake
8

For example, in our project, #include <QtCrypto> is the only one that requires the NOMINMAX macro. Therefore, if QtCrypto is the source of the problem, then it should be solved on the QCA side.
If some other project use windows.h in public headers consumer must set the NOMINMAX, it is overly complicated e.g. for project that you can't change, is it?

anikolaev planned changes to this revision.Nov 13 2018, 7:06 PM
anikolaev marked 2 inline comments as done.
sitter removed a reviewer: sitter.Aug 6 2019, 9:42 AM