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

[Chris Hegarty]

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