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

[Chris Hegarty]

Socket.java has some commented out code. I think it should just be 
removed, right?

I'm guessing this is the only change to the previous webrev? If so, it 
should be good to go. I don't think you need to re-generate a webrev for 
the above trivial change.

-Chris

On 09/16/11 06:17 PM, Kurchi Hazra wrote:
> 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
>>>>>>>
>>>
>