[OpenJDK 2D-Dev] JDK 9 RFR of JDK-8033624: Fix raw and unchecked lint warnings in sun.font

[Phil Race]

OK, modulo the formatting I have no further comments.
Some day we may revisit (some of) the code that was tricky to re-write ..

-phil.

On 2/7/2014 12:30 AM, Joe Darcy wrote:
> Hello,
>
> On 02/06/2014 04:13 PM, Phil Race wrote:
>> Comments/questions on the changes :-
>>
>> Font2D.java
>>
>>
>> 108     protected Reference<FontStrike> lastFontStrike = new 
>> SoftReference<>(null);
>> ...
>>
>>
>> 320 lastFontStrike = new SoftReference<>(strike);
>>
>> So the diamond operator can be used in this 2nd case ? Apparently so.
>
> Yes; I verified the modified code successfully compiled :-)
>
>> I vaguely remember that it used to only be able to infer the type in 
>> the first case ?
>> Did it get updated or did I just not realise ??
>
> In JDK 7, the compiler could often infer the right type for a diamond 
> in most contexts, but in JDK 8, the inference scheme is more powerful. 
> IIRC, the main difference is that in JDK 8 the compiler can do a 
> better job of inferring diamond when it occurs in a position like an 
> argument in a method call (where there is an interaction between the 
> inference of diamond and the overload resolution logic).
>
>>
>>
>> There are a few places including these :-
>> -----
>> AttributeValues.java
>>
>>  Map<TextAttribute, ?> imStyles = hl.getStyle();
>>
>> FontResolver.java
>>
>>
>> 226     public Font getFont(int index,
>>  227                         Map<? extends 
>> AttributedCharacterIterator.Attribute, ?> attributes) {
>>
>> ----
>>
>> where its not obvious how you decided the type to use.
>
> In that particular case, I looked at the return type of the method 
> java.awt.Font.getAttributes(); I did the analogous looking in the 
> other case.
>
>> I know a lot of this code and that they look reasonable but I 
>> wouldn't have been confident
>> that there weren't places where we allowed some wider type to be used 
>> in these maps.
>> How did you decide on the types and how did you verify it ?
>
> The verification occurs by compiling the JDK; if there was a mismatch 
> the compiler would compile. (Since these are sun.* classes, they are 
> officially only supposed to be used inside the JDK.)
>
>>
>>
>>>      @SuppressWarnings("unchecked")
>>
>> Can you show what thevarious  warnings looked like and say why they 
>> needed to be suppressed rather than fixed ?
>
> In AttributeValues, the warning in the original code is:
>
> src/share/classes/sun/font/AttributeValues.java:822: warning: 
> [unchecked] unchecked cast
>                 av = AttributeValues.fromMap((Map<Attribute, ?>)map); 
> // yuck
>                                                                 ^
>   required: Map<Attribute,?>
>   found:    Map<CAP#1,CAP#2>
>   where CAP#1,CAP#2 are fresh type-variables:
>     CAP#1 extends Object from capture of ?
>     CAP#2 extends Object from capture of ?
>
> The map value comes in as a parameter of type Map<?, ?> map and then 
> some runtime check verify it is an AttributeMap. This these two 
> unchecked casts are isolated, I didn't think it was worthwhile to try 
> to perform more extensive surgery to try to fix up the types.
>
>> Is it anywhere you found it to tricky/impossible to get rid of a cast ?
>
> Yes, largely where @SuppressWarning("unchecked") was used :-) One 
> example, in
>
> +            @SuppressWarnings("unchecked")
>              HashMap<String,String> ffmapCopy =
> (HashMap<String,String>)(fontToFileMap.clone());
>
> the unchecked suppression is needed because the clone method in 
> HashMap is declared to return "Object" rather than "HashMap<K, V>" 
> even though it does actually return a "HashMap<K, V>". I looked into 
> changing this in collections, but there are subtle compatibility 
> issues that unfortunately argue for leaving these particular clone 
> methods as returning Object.
>
> There are a few occurrences of casting the result of cloning a 
> collection.
>
> The appContext mechanism is not fully generics friendly and needed 
> some casts and unchecked suppression.
>
> Some code in SunFontManager.java attempted to make generics and arrays 
> play together in an unnatural way and @SuppressWarning("unchecked") 
> was needed to keep the peace without attempting a larger rewrite.
>
>>
>> FontScaler.java :
>>
>> Is the whole somewhat ugly re-writing of this so that you have a 
>> single expression
>> to which to apply  @SuppressWarnings ?
>
> Basically, but the situation is worse than usual since Class.forName 
> maps a String to a Class<?> and I wanted only one cast to (Class<? 
> extends FontScaler>) with a single @SuppressWarnings for that cast.
>
>>
>> If you are going to do that anyway was it worth it ?  Can't it just 
>> be applied
>> to the static initialiser block ?
>
> The SuppressWarning declaration cannot be applied to the static 
> initializer block since the block is not the sort of declaration that 
> can host an annotation.
>
>>
>> And at the very least there should be spaces before "?" and ":" to 
>> make it
>> easier to read.
>
> Okay.
>
>>
>>
>> StrikeCache.java:
>>
>> Disposer.addReference((Reference<Object>)(Reference)this, disposer);
>>
>> Why does this look like a cast of an already cast variable ? Looks 
>> weird to me.
>
> The short answer is that a cast through a raw type, "Reference" in 
> this case, is sometimes necessary to convince the compiler you can do 
> something it doesn't think you should be able to do. The "this" value 
> has a type of Reference<FontStrike> and that is not directly 
> convertible to Reference<Object>. (The Disposer is weirdly typed, 
> basically a type-hetrogenous container, and would have to be more 
> extensively revised to play well with generics.)
>
>>
>>
>> SunFontManager.java
>>
>> lines 2310-2314 look much > 80 chars. We keep all these files <=80 
>> chars wide
>>
>> I find it tricky to catch all those in review but please be conscious 
>> of it.
>> I suspect line 2644 in the same file is also too long.Also 3066, 
>> 3068, 3128, 3157..
>> please fix all of these and any others I may have overlooked in this 
>> or other files.
>
> Okay.
>
>>
>> And don't forget - please re-generate against client, or ask me to 
>> push there for you.
>>
>> -phil.
>>
>
> Thanks for the review,
>
> -Joe
>