> On 27 Sep 2016, at 19:56, Langer, Christoph <christoph.lan...@sap.com> wrote: > > Chris, > > thanks for your input. > > If there's no objections I'd push it like this later tomorrow: > http://cr.openjdk.java.net/~clanger/webrevs/8166584.2/
Looks ok to me Christoph. Thanks, -Chris. > I've replaced the JNU_JAVANETPKG and JNU_JAVAIOPKG macros with the full > exception class names. > > Best regards > Christoph > >> -----Original Message----- >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] >> Sent: Dienstag, 27. September 2016 10:10 >> To: Langer, Christoph <christoph.lan...@sap.com> >> Cc: net-dev@openjdk.java.net >> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function >> NET_ThrowSocketException in windows libnet >> >> Christoph, >> >> On 26 Sep 2016, at 18:58, Langer, Christoph <christoph.lan...@sap.com> >> wrote: >>> >>> Hi Chris, >>> >>> I agree with your comment on the NPE. It would probably be wrong. So I >> restored the old code and also removed the comments suggesting the NPE. >> Here is my new webrev: >> http://cr.openjdk.java.net/~clanger/webrevs/8166584.1/ >> >> This looks fine. >> >>> What I was thinking a bit more about after I posted my initial webrev was >>> the >> fact that the old NET_ThrowSocketException would register a GlobalRef to >> java/net/SocketException whereas the other, more generic code would always >> use the lookup by name. Would you think it is a performance benefit to keep a >> reference to a standard exception class in some place and use it for throwing >> or is it better to always look up the class? Throwing those kind of >> exceptions is >> probably not on the hot path anyway - but on the other hand it should be no >> issue to keep references to these very basic class types. What's your view on >> that? >> >> I don’t believe that using a GlobalRef is worth it here. It adds a little >> complication, while not offering much benefit. JNU_ThrowByName >> should be fine. >> >>> And another probably aesthetic thing: I notice that sometimes a >> JNU_JAVANETPKG "SocketException" is used and sometimes a >> "java/net/SocketException", even within the same file like >> SocketInputStream.c. >> Maybe I should unify this in the files that I touch here and if yes, shall I >> use the >> literal name or the JNU_JAVANETPKG define? Any opinion on that? >> >> My preference is to remove JNU_JAVANETPKG, and just use "java/net/“. >> >> -Chris >> >>> Thanks for taking care of this, >>> Christoph >>> >>> >>>> -----Original Message----- >>>> From: Chris Hegarty [mailto:chris.hega...@oracle.com] >>>> Sent: Montag, 26. September 2016 16:51 >>>> To: Langer, Christoph <christoph.lan...@sap.com>; net- >> d...@openjdk.java.net >>>> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function >>>> NET_ThrowSocketException in windows libnet >>>> >>>> Christoph, >>>> >>>> On 22/09/16 21:59, Langer, Christoph wrote: >>>>> Hi, >>>>> >>>>> while looking at utility functions for creating exceptions in >>>>> libjava/libnet I found a small spot that should be consolidated right >>>>> away. >>>>> >>>>> >>>>> The function NET_ThrowSocketException does only exist in the windows >>>>> native implementation and is only used in 3 places in >>>>> SocketInputStream.c. I removed this in favor of directly calling >>>>> JNU_ThrowByName as the Unix variant of that code already does. >>>>> >>>>> >>>>> In that function Java_java_net_SocketInputStream_socketRead0 I also >>>>> replaced throwing a SocketException with throwing an NPE in the rare >>>>> case that a the JNI input for the file descriptor is null. That's >>>>> probably more natural and should virtually never occur anyways. >>>> >>>> Hmmm... I'm not sure about this. SocketException is thrown on >>>> unix too for a similar situation. More significantly, a null value >>>> represents that the socket has been, possibly asynchronously, >>>> closed. >>>> >>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 >>>>> >>>>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ >>>> >>>> Other than the above concern, the remainder of the code looks ok >>>> to me. >>>> >>>> -Chris. >
