> 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-dev@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 >
