Bug 73563 – use NOMINMAX instead of #define min min

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Paul]

This is a patch from my code.  I'm working with QT 4.7.4,
but I've checked webkit's code on the trunk via trac's Source Browse and I see the problem still exists.

Instead of using #define min min and #define max max, to avoid problems with windef.h
just use #define NOMINMAX

This is actually what is done in wtf/Platform.h although I think that only applies for WINCE.



=== modified file 'qt/src/3rdparty/javascriptcore/JavaScriptCore/config.h'
--- qt/src/3rdparty/javascriptcore/JavaScriptCore/config.h      2011-10-08 14:07:42 +0000
+++ qt/src/3rdparty/javascriptcore/JavaScriptCore/config.h      2011-12-01 15:14:46 +0000
@@ -41,8 +41,7 @@
 
 // If we don't define these, they get defined in windef.h. 
 // We want to use std::min and std::max
-#define max max
-#define min min
+#define NOMINMAX       /* Windows min and max conflict with standard macros */
 
 #if !COMPILER(MSVC7) && !OS(WINCE)
 // We need to define this before the first #include of stdlib.h or it won't contain rand_s.

[Alexey Proskuryakov]

Would you be willing to submit a patch, as described in <http://www.webkit.org/coding/contributing.html>?

Please correct the comment above, too.

[Paul]

Created attachment 117817 [details]
Patch

[Paul]

Uploaded patch that adjusts 5 header files to declare NOMINMAX instead of using the trick #define min min
This is the more correct way of dealing with windef.h's #define min/max issue on the windows platform.

I have not tested it, as I am not set up to build Webkit, sorry.  If I need to make more significant changes in the future then I'll go through that process.


Also, I'm not sure how to edit previously made comments.

[WebKit Review Bot]

Comment on attachment 117817 [details]
Patch

Attachment 117817 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10729611

New failing tests:
svg/custom/linking-uri-01-b.svg

[Paul]

I don't see how my patch could've generated an image mismatch in the test suite,

I can only assume it was broken before the patch was applied... can that be the case?

[Julien Chaffraix]

> I can only assume it was broken before the patch was applied... can that be the case?

It's a flaky test (cf webkit-dev) and is likely not your wrong doing.

[Adam Roben (:aroben)]

Comment on attachment 117817 [details]
Patch

I agree this is better than #define min min!

I think an even better way to do this these days would be to put this in WebKitLibraries/win/tools/vsprops/common.vsprops. All projects inherit settings from there.

[Patrick R. Gansterer]

Created attachment 212218 [details]
Patch

[Early Warning System Bot]

Comment on attachment 212218 [details]
Patch

Attachment 212218 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/2034010

[Early Warning System Bot]

Comment on attachment 212218 [details]
Patch

Attachment 212218 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/2031013

[Patrick R. Gansterer]

Created attachment 212221 [details]
Patch

[Brent Fulgham]

Comment on attachment 212221 [details]
Patch

This looks good.  I wish the EWS had been working when you filed the bug, but I don't see any obvious reason for this to fail.

Let's land it and I'll roll it out if there is any build problem.

[Patrick R. Gansterer]

Comment on attachment 212221 [details]
Patch

Clearing flags on attachment: 212221

Committed r156302: <http://trac.webkit.org/changeset/156302>

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.

[Patrick R. Gansterer]

*** Bug 109161 has been marked as a duplicate of this bug. ***