Code Review Request: 7090158 Networking Libraries don't build with javac -Werror (original) (raw)

Kurchi Hazra kurchi.subhra.hazra at oracle.com
Fri Sep 16 10:17:55 PDT 2011

Updated webrev:

http://cr.openjdk.java.net/~sherman/kurchi/7090158/webrev/

Thanks, Kurchi

On 9/14/2011 12:42 PM, chris hegarty wrote: > Kurchi, >> The problem here is that due to a bug in the compiler, CR 7090499, we > are not seeing raw type warnings for anonymous inner classes. >> When Maurizio fixes this bug it is very likely that other areas of the > jdk where Sasha enabled -Werror will break the build. >> If you like you can complete what you have in the networking area and > we can file a separate CR for this new issue. But we should make it a > priority. >> Tom: > >.... > >Or better, remove cl and change: > > > > clazz.getDeclaredMethod("connect", cl); > > > >to: > > clazz.getDeclaredMethod( > > "connect", SocketAddress.class, int.class > > ); >> Thanks Tom, this suggestion is much better. Kurchi we should probably > run with this. >> -Chris. >> On 14/09/2011 16:46, Kurchi Hazra wrote: >> I remember having made this change as follows: >> -bash-3.00$ hg diff src/share/classes/java/net/Socket.java >> diff --git a/src/share/classes/java/net/Socket.java >> b/src/share/classes/java/net/Socket.java >> --- a/src/share/classes/java/net/Socket.java >> +++ b/src/share/classes/java/net/Socket.java >> @@ -459,10 +459,10 @@ class Socket implements java.io.Closeabl >> oldImpl = AccessController.doPrivileged >> (new PrivilegedAction() { >> public Boolean run() { >> - Class[] cl = new Class[2]; >> - cl[0] = SocketAddress.class; >> - cl[1] = Integer.TYPE; >> - Class clazz = impl.getClass(); >> + Class<?>[] cl = new Class[2]; >> + cl[0] = SocketAddress.class; >> + cl[1] = Integer.TYPE; >> + Class<?> clazz = impl.getClass(); >> while (true) { >> try { >> clazz.getDeclaredMethod("connect", cl); >>>>>> I am not sure why webrev is not picking these changes, but these >> warnings did show up. >>>>>> Thanks, >> Kurchi >>>> On 9/14/2011 7:11 AM, Chris Hegarty wrote: >>> Maurizio and I noticed another issue in java.net.Socket >>>>>> > Class<?>[] cl = new Class[2]; >>>>>> should generate a warning, but does not. Maurizio filed CR 7090499 >>> against this. It is a compiler bug. >>>>>> It should be: Class[] cl = new Class[2]; >>>>>> It is best that we fix this before pushing as it will break the build >>> if we wait until after 7090499 is fixed. Also we may be better off >>> waiting until after Maurizio runs a JPRT job with his fix for 7090499. >>> We may have other failures in areas where Sasha enabled warnings. >>>>>> -Chris. >>>>>> On 14/09/2011 14:17, Chris Hegarty wrote: >>>> Thanks for reviewing this Max, I also went through the changes. >>>>>>>> MessageHeader.filterAndAddHeaders() and >>>> HttpURLConnection.getRequestproperties() also confused me. Essentially >>>> this never work quite right. I had Maurizio ( javac guy ) look at the >>>> old code with me and we figured out that the generic type on >>>> filterAndAddHeaders was never enforced by the compiler as it was being >>>> passed a raw Map. >>>>>>>> The code in filterAndAddHeaders assumes that the values in the map are >>>> Strings even though the type is declared as List. Again this >>>> cannot be enforced as the only client of filterAndAddHeaders is >>>> passing >>>> a raw Map. Anyway, it worked somehow! >>>>>>>> What Kurchi has now looks right to me. >>>>>>>> -Chris. >>>>>>>> On 14/09/2011 06:22, Weijun Wang wrote: >>>>>>>>>>>>>>> On 09/14/2011 12:14 PM, Kurchi Hazra wrote: >>>>>> Updated webrev : >>>>>> http://cr.openjdk.java.net/~weijun/7090158/webrev.00/. >>>>>> This should build correctly. >>>>>>>>>> Yes, it does! >>>>>>>>>> Some comments: >>>>>>>>>> 1. make/java/Makefile has no real change >>>>>>>>>> 2. make/javax/others/Makefile has only a new commented line >>>>>>>>>> 3. java/net/DatagramSocket.java and java/net/MulticastSocket.java >>>>> have >>>>> some real code changes around bind(). Maybe they should go to another >>>>> fix? >>>>>>>>>> 4. sun/net/www/MessageHeader.java: >>>>>>>>>> 231 for(Map.Entry<String, List> entry : include.entrySet()) { >>>>>>>>>> There should be a blank between "for" and "(", but probably not >>>>> necessary between "String," and "List" or "entry" and ":". You >>>>> decide. >>>>>>>>>> 234 l = new ArrayList<>(); >>>>>>>>>> Do we have a consensus on whether diamond can be used here? i.e. >>>>> assignment not on declaration. >>>>>>>>>> Another thing: >>>>>>>>>> I'm confused by the use of MessageHeader.filterAndAddHeaders() inside >>>>> HttpURLConnection.getRequestproperties() methods. Looking at the old >>>>> codes, it seems the userCookiesMap (in >>>>> HttpURLConnection.getRequestproperties) variable is only a >>>>> Map<String,String>, but the 2nd argument of filterAndAddHeaders() has >>>>> been declared as Map<String,List> for some time, then again, >>>>> the >>>>> old filterAndAddHeaders() calls "l.add(entry.getValue())" which >>>>> suggests >>>>> the value of the map is still only String. >>>>>>>>>> My current understanding is that both the old codes and Kurchi's code >>>>> changes work but the old one's method declaration is not correct. >>>>> Also, >>>>> if the include argument never contains multiple (or empty) string >>>>> values >>>>> for the same key, we can simply use Map<String,String>. >>>>>>>>>> Thanks >>>>> Max >>>>>>>>>>>>>>>>>>>>>> Thanks, >>>>>> Kurchi >>>>>>>>>>>>>>>>>> On 9/13/2011 7:55 PM, Weijun Wang wrote: >>>>>>> I apply the patch to my local repository and do a clean rebuild of >>>>>>> jdk-only. It shows 1 error and 92 warnings in javax and stopped. >>>>>>> Most >>>>>>> in src/share/classes/javax/xml/crypto/dsig and I remember Sean said >>>>>>> it's not easy to remove all warnings there because the codes are >>>>>>> shared between JDK and some Apache projects. >>>>>>>>>>>>>> -Max >>>>>>>>>>>>>> On 09/14/2011 03:13 AM, Alan Bateman wrote: >>>>>>>> Kurchi Hazra wrote: >>>>>>>>> Something went wrong in the pasting. Can you check if this works >>>>>>>>> fine: >>>>>>>>> http://cr.openjdk.java.net/~chegar/7090158/webrev/ >>>>>>>> Yes, this webrev has what I expected to see. >>>>>>>>>>>>>>>> -Alan >>>>>>>>

-Kurchi

More information about the net-dev mailing list