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

[Joe Darcy]

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