Bug 88313 – [Qt][Win] Fix build fails because of min/max macros and std::numeric_limits<*>::min/max() clashing

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Csaba Osztrogonác]

qwebframe.cpp, ... doesn't build because of this problem.

[Csaba Osztrogonác]

Created attachment 145809 [details]
Patch

[Csaba Osztrogonác]

Created attachment 145813 [details]
Patch

Typo fixed. Thanks GTK EWS. :)

[Simon Hausmann]

Comment on attachment 145813 [details]
Patch

This is like a sledge-hammer :). Is it perhaps cleaner to do NOMINMAX only in those .cpp files where the clash occurs? That seems to be the more common solution and I think preferred if for example it only happens in qwebframe.cpp.

[Csaba Osztrogonác]

(In reply to comment #3)
> (From update of attachment 145813 [details])
> This is like a sledge-hammer :). Is it perhaps cleaner to do NOMINMAX only in those .cpp files where the clash occurs? That seems to be the more common solution and I think preferred if for example it only happens in qwebframe.cpp.

:) As far as I remember we need this for 5-10 files not only one.
Let me check how many files need it.

[Csaba Osztrogonác]

problematic source files:
- Source/WebKit/qt/Api/qwebview.cpp
- Source/WebKit/qt/Api/gqraphicswebview.cpp
- Source/WebKit/qt/Api/qwebpage.cpp
- Source/WebKit/qt/Api/qwebhistory.cpp
- Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp

Patch is coming soon.

[Csaba Osztrogonác]

Created attachment 146300 [details]
Patch

Add NOMINMAX only to problematic sources instead of using sledgehammer :)

[Balazs Kelemen]

(In reply to comment #6)
> Created an attachment (id=146300) [details]
> Patch
> 
> Add NOMINMAX only to problematic sources instead of using sledgehammer :)

In most parts of the project we already use the sledgehammer approach.
In config.h's you can find this - under #if OS(WINDOWS)

// If we don't define these, they get defined in windef.h.
// We want to use std::min and std::max.
#ifndef max
#define max max
#endif
#ifndef min
#define min min
#endif

What I don't know is that which config.h is used in Source/WebKit/qt and why it does not have this. Furthermore, adding NOMINMAX seems more elegant than this ifdef hackery but I'm not sure it would work on every platform. I favor fixing it in the appropriate config.h for consistency.

[Jocelyn Turcotte]

WebCore/config.h is used by WebKit/qt.

We "#define max max" correctly in config.h but the problem is that qdatetime.h does "#undef max" before WinDef.h gets included.

How the hell did this end up in a public Qt header...

[Simon Hausmann]

(In reply to comment #8)
> WebCore/config.h is used by WebKit/qt.
> 
> We "#define max max" correctly in config.h but the problem is that qdatetime.h does "#undef max" before WinDef.h gets included.
> 
> How the hell did this end up in a public Qt header...

Well spotted!! And pretty recently changed, too:

https://codereview.qt-project.org/#change,13166

The header file uses std::min/max in an inline function, but yeah, the fix is there IMHO.

[Simon Hausmann]

Work in progress at https://codereview.qt-project.org/#change,28549 - If we can't get that into Qt, then we have to change the existing workaround in WebKit to use NOMINMAX instead of #define min min/max max

[Csaba Osztrogonác]

(In reply to comment #10)
> Work in progress at https://codereview.qt-project.org/#change,28549 - If we can't get that into Qt, then we have to change the existing workaround in WebKit to use NOMINMAX instead of #define min min/max max

I tried it, it works for me without workaround for WebKit. ;-)

[Simon Hausmann]

(In reply to comment #11)
> (In reply to comment #10)
> > Work in progress at https://codereview.qt-project.org/#change,28549 - If we can't get that into Qt, then we have to change the existing workaround in WebKit to use NOMINMAX instead of #define min min/max max
> 
> I tried it, it works for me without workaround for WebKit. ;-)

Great. The change is slowly making its way into Qt. Most of the submodules are patched already and the qtbase change is staged (after the n-th try :)

Let's close this as INVALID then, because the root cause for this bug is an upstream issue that disabled the existing workaround.

[Simon Hausmann]

Comment on attachment 146300 [details]
Patch

Clearing review