Code Review Request: 7090158 Networking Libraries don't build with javac -Werror

[Kurchi Hazra]

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<Boolean>() {
>> 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<String>. 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<String>> 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<String>> 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