Bug 109161 - [WIN] Use NOMINMAX consistently
Summary: [WIN] Use NOMINMAX consistently
Status: RESOLVED DUPLICATE of bug 73563
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Hausmann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-07 02:43 PST by Simon Hausmann
Modified: 2013-10-30 15:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.45 KB, patch)
2013-02-07 02:47 PST, Simon Hausmann
no flags Details | Formatted Diff | Diff
Patch (8.46 KB, patch)
2013-02-07 03:07 PST, Simon Hausmann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2013-02-07 02:43:18 PST
[WIN] Use NOMINMAX consistently
Comment 1 Simon Hausmann 2013-02-07 02:47:35 PST
Created attachment 187034 [details]
Patch
Comment 2 Early Warning System Bot 2013-02-07 03:05:09 PST
Comment on attachment 187034 [details]
Patch

Attachment 187034 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16431080
Comment 3 Simon Hausmann 2013-02-07 03:07:32 PST
Created attachment 187039 [details]
Patch
Comment 4 Patrick R. Gansterer 2013-02-07 03:59:52 PST
Since it's a duplicate of https://bugs.webkit.org/show_bug.cgi?id=73563, please look the the commtents there. I'd define it via buildsystem and remove it from all config.h files
Comment 5 Jocelyn Turcotte 2013-02-07 05:42:00 PST
(In reply to comment #4)
> Since it's a duplicate of https://bugs.webkit.org/show_bug.cgi?id=73563, please look the the commtents there. I'd define it via buildsystem and remove it from all config.h files

I'm sure Adam had a good reason behind his comment, but since we would have at least 3 build systems to adjust for this I would prefer to keep it in config.h.

I believe that config.h should define what the code expects/needs from a platform, while the build system should take care of smaller details of the configuration and system the binary will run on.
Comment 6 Jocelyn Turcotte 2013-02-07 05:46:30 PST
Though there is already a NOMINMAX definition in:
Source/JavaScriptCore/JavaScriptCore.vcproj/testapi/testapiCommon.vsprops
Source/JavaScriptCore/JavaScriptCore.vcxproj/testapi/testapiCommon.props
Source/cmake/OptionsWindows.cmake
Tools/DumpRenderTree/win/ImageDiffCommon.vsprops
Tools/DumpRenderTree/win/ImageDiffWinCairoCommon.vsprops

Simon, what do you think?
Comment 7 Patrick R. Gansterer 2013-02-07 05:50:42 PST
(In reply to comment #5)
> I believe that config.h should define what the code expects/needs from a platform, while the build system should take care of smaller details of the configuration and system the binary will run on

if we have _ONE_ config.h i agree, but since that's not the case i'd prefere the build system option. Otherwise you should move the other defines into the config.h files too.
Comment 8 Laszlo Gombos 2013-02-07 06:05:18 PST
Can this code be moved to Platform.h ? If it can that I find Platform.h a good place for it.
Comment 9 Simon Hausmann 2013-02-07 06:06:36 PST
I don't mind trying the build system option then. That would mean removing the #define min from config.h altogether.
Comment 10 Laszlo Gombos 2013-02-07 06:15:43 PST
(In reply to comment #9)
> I don't mind trying the build system option then. That would mean removing the #define min from config.h altogether.

Unless this is a technical necessity for the Window platform/build system, moving this definition to the build system is not the general direction that the project seems to prefer. I similar (bot not exactly the same) matter - see bug 108191 .
Comment 11 Patrick R. Gansterer 2013-02-07 06:21:44 PST
(In reply to comment #10)
> (In reply to comment #9)
> > I don't mind trying the build system option then. That would mean removing the #define min from config.h altogether.
> 
> Unless this is a technical necessity for the Window platform/build system, moving this definition to the build system is not the general direction that the project seems to prefer. I similar (bot not exactly the same) matter - see bug 108191 .

The difference is, that in that case you define a variable, which isn't used outside of WebKit, but NOMINMAX is required for a system header, which can be included at any time, where Platform.h can be too late.
I completely agree that the build system isn't the best direction, but unless we have a "commonconfig.h", which gets includes by all other config.h, I still prefere the build system option (for now).
Comment 12 Brent Fulgham 2013-10-30 10:40:19 PDT
Comment on attachment 187039 [details]
Patch

I don't think this patch is needed anymore, now that NOMINMAX has been added to the build property sheets.
Comment 13 Patrick R. Gansterer 2013-10-30 15:17:19 PDT

*** This bug has been marked as a duplicate of bug 73563 ***