Ping: RFR (M): 8167481: cleanup of headers and includes for native libnet

[Chris Hegarty]

> On 20 Oct 2016, at 16:30, Langer, Christoph <christoph.langer at 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.hegarty at oracle.com]
>> Sent: Dienstag, 18. Oktober 2016 20:56
>> To: Langer, Christoph <christoph.langer at sap.com>; net-dev at 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.langer at 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
>