Thanks, Chris. I pushed it...
> -----Original Message-----
> From: Chris Hegarty [mailto:chris.hega...@oracle.com]
> Sent: Mittwoch, 26. Oktober 2016 15:39
> To: Langer, Christoph <christoph.lan...@sap.com>
> Cc: net-dev@openjdk.java.net
> Subject: Re: Ping: RFR (M): 8167481: cleanup of headers and includes for
> native
> libnet
>
>
> > On 20 Oct 2016, at 16:30, Langer, Christoph <christoph.lan...@sap.com>
> wrote:
> >
> > Chris,
> >
> > I created a new version that addresses your points:
> > http://cr.openjdk.java.net/~clanger/webrevs/8167481.1/
>
> Looks good to me.
>
> > I reordered the headers according to your suggestions ( 1) system, 2)
> platform specific includes, 3) JNI includes, and finally, 4) generated
> headers ),
> added the @Native and removed the Windows stuff in net_util_md.h. To make
> reodering of system includes possible, I had to add back some system headers
> in a few places and could not always maintain the alphabetical order.
>
> Ok, thanks.
>
> > I think you should run it through PRT and then hopefully give me your final
> blessing…
>
> I ran your patch through our internal build and test system, and there are no
> surprises. Consider is Reviewed, at least by me.
>
> -Chris.
>
> > Thanks & Best regards
> > Christoph
> >
> >> -----Original Message-----
> >> From: Chris Hegarty [mailto:chris.hega...@oracle.com]
> >> Sent: Dienstag, 18. Oktober 2016 20:56
> >> To: Langer, Christoph <christoph.lan...@sap.com>; net-
> d...@openjdk.java.net
> >> Subject: Re: Ping: RFR (M): 8167481: cleanup of headers and includes for
> native
> >> libnet
> >>
> >> Christoph,
> >>
> >>> On 17 Oct 2016, at 09:40, Langer, Christoph <christoph.lan...@sap.com>
> >> wrote:
> >>> …
> >>> Hi,
> >>>
> >>> as I already mentioned, here is another proposal for cleanup in libnet:
> >>> https://bugs.openjdk.java.net/browse/JDK-8167481
> >>> http://cr.openjdk.java.net/~clanger/webrevs/8167481.0/
> >>
> >> Overall, this looks good. Specific comments below ...
> >>
> >>> This time I touched the includes. Generally I reordered the includes to
> include
> >> “net_util.h” first, then any JNI includes, such as “java_net_InetAddress.h”
> and
> >> finally all standard header includes in alphabetical order. For platform
> specific
> >> includes, I use the order Linux, AIX, Solaris, BSD. I removed all includes
> >> that
> >> don’t seem to be necessary to get the respective file compiled. Is that the
> >> correct approach that is desired?
> >>
> >> It is good to be consistent. My personal preference is to order the
> >> includes;
> >> 1) system, 2) platform specific includes, 3) JNI includes, and finally, 4)
> >> generated headers ( e.g. “java_net_InetAddress.h” ).
> >>
> >>> To make this work, I had to remove the definitions “IPv4” and “IPv6” in
> >> net_util.h and replace their usages with “java_net_InetAddress_IPv4” resp.
> >> “java_net_InetAddress_IPv6” from JNI which seems to be the correct thing
> to
> >> do anyway.
> >>
> >> Right. This is a good change. Trivially, and it is not strictly necessary
> >> but
> >> good for readability, would be to add @Native to InetAddress.IPv4 and
> >> InetAddress.IPv6.
> >>
> >>> For Windows I also cleaned up (removed) the definition of SOCKADDR_IN6
> in
> >> net_util_md.h and replaced all occurences with sockaddr_in6. It seems that
> the
> >> capital letters version SOCKADDR_IN6 is a remainder of a very old Microsoft
> >> runtime (_MSC_VER < 1310) which corresponds to MSVC 7.0, which is even
> >> older than Visual Studio 2003. So I’m wondering if I should remove the
> whole
> >> section in windows net_util_md.h now (lines 47 – 177)?
> >>
> >> I think it should be ok to remove this.
> >>
> >>> For Windows it seems that we can also completely get rid of
> >> src/java.base/windows/native/libnet/icmp.h as the icmp stuff is all brought
> by
> >> the #include <icmpapi.h>. Would you agree to that?
> >>
> >> Ok.
> >>
> >>> So far I successfully ran builds in my local environments for Windows,
> Linux,
> >> AIX, Solaris and Apple. I’ve now added the changeset to our local
> >> regression
> >> testing environment for OpenJDK. If the change is to your like, I would
> >> like to
> >> ask you to also run it through JPRT to see if I missed some dependency.
> >>
> >> I ran your patch through our internal build and test system and there
> >> are no surprises.
> >>
> >> -Chris.
> >>
> >>> Thanks and best regards
> >>> Christoph
> >
