Hi David, Thanks for reviewing! I’ll delay progressing this one a bit until 8199619 is integrated.
Best regards, Robin > On 27 Mar 2018, at 02:57, David Holmes <david.hol...@oracle.com> wrote: > > Looks good to me. > > Thanks, > David > > On 27/03/2018 1:01 AM, Robin Westberg wrote: >> Hi David, >> Thanks for taking a look! >>> On 26 Mar 2018, at 01:03, David Holmes <david.hol...@oracle.com >>> <mailto:david.hol...@oracle.com>> wrote: >>> >>> Hi Robin, >>> >>> On 23/03/2018 10:37 PM, Robin Westberg wrote: >>>> Hi Kim & Erik, >>>> Certainly makes sense to define it from the build system, I’ve updated the >>>> patch accordingly: >>>> Full: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/ >>>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01/> >>>> Incremental: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/ >>>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00-01/> >>> >>> I'm a little unclear on the hotspot changes. If we define >>> WIN32_LEAN_AND_MEAN then certain APIs like sockets are excluded from >>> windows.h so we then have to include the specific header files like >>> winsock2.h - is that right? >> Yep that’s correct, headers like winsock, dde, ole, shellapi and a few other >> uncommon ones are no longer included from windows.h when this is defined. >>> src/hotspot/share/interpreter/bytecodes.cpp >>> >>> I'm curious about this change. u_short comes from types.h on non-Windows, >>> is it simply missing on Windows (at least once we have WIN32_LEAN_AND_MEAN >>> defined) ? >> Yeah, on Windows these comes from winsock(2).h: >> /* >> * Basic system type definitions, taken from the BSD file sys/types.h. >> */ >> typedef unsigned char u_char; >> typedef unsigned short u_short; >> typedef unsigned int u_int; >> typedef unsigned long u_long; >> I noticed that one of these (u_char) is also defined in >> globalDefinitions.hpp so could perhaps define u_short there, or include >> winsock2.h globally again. But since it was only used in a single place in >> the existing code it seemed simple enough to just expand the typedef there. >>> src/hotspot/share/utilities/ostream.cpp >>> >>> 1029 #endif >>> 1030 #if defined(_WINDOWS) >>> >>> Using elif could be marginally faster given the two sets of conditions are >>> mutually exclusive. >> Good point, will change it. >> I also had to move the flag definition to adapt to the latest changes in the >> hs repo, cc’ing build-dev again to make sure I got it right. >> Updated webrev (full): >> http://cr.openjdk.java.net/~rwestberg/8199736/webrev.02/ >> Updated webrev (incremental): >> http://cr.openjdk.java.net/~rwestberg/8199736/webrev.01-02/ >> Best regards, >> Robin >>> >>> Thanks, >>> David >>> >>>> (Not quite sure if the definition belongs where I put it or a bit later >>>> where most other windows-specific JVM flags are defined, but seemed >>>> reasonable to put it close to where it is defined for the JDK libraries). >>>> Best regards, >>>> Robin >>>>> On 22 Mar 2018, at 16:52, Kim Barrett <kim.barr...@oracle.com >>>>> <mailto:kim.barr...@oracle.com>> wrote: >>>>> >>>>>> On Mar 22, 2018, at 10:34 AM, Robin Westberg <robin.westb...@oracle.com >>>>>> <mailto:robin.westb...@oracle.com>> wrote: >>>>>> >>>>>> Hi all, >>>>>> >>>>>> Please review the following change that defines WIN32_LEAN_AND_MEAN [1] >>>>>> before including windows.h. This marginally improves build times, and >>>>>> makes it possible to include winsock2.h. >>>>>> >>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8199736 >>>>>> <https://bugs.openjdk.java.net/browse/JDK-8199736> >>>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/ >>>>>> <http://cr.openjdk.java.net/~rwestberg/8199736/webrev.00/> >>>>>> Testing: hs-tier1 >>>>>> >>>>>> Best regards, >>>>>> Robin >>>>>> >>>>>> [1] >>>>>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx#faster_builds_with_smaller_header_files >>>>>> >>>>>> <https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx#faster_builds_with_smaller_header_files> >>>>> >>>>> I think the addition of the WIN32_LEAN_AND_MEAN definition should be done >>>>> through the build >>>>> system, so that it applies everywhere. >>>>>
