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/ 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.
