<AWT Dev> JDK 9 RFR of JDK-8039109 : Fix unchecked and raw lint warnings in java.awt

[Remi Forax]

On 05/01/2014 01:04 AM, Joe Darcy wrote:
> Hi Rémi,
>
>> Hi Joe,
>>
>> in AWTKeyStroke, instead of
>>
>> Class<AWTKeyStroke> clazz = 
>> (Class<AWTKeyStroke>)AppContext.getAppContext().get(AWTKeyStroke.class);
>>
>> I should have the right type Class<? extends ...> (the class is a
>> subclass of AWTKeyStroke) and
>> do a classcheck at runtime when the class is extracted instead of later
>>
>> Class<? extends AWTKeyStroke> clazz = 
>> ((Class<?>)AppContext.getAppContext().get(AWTKeyStroke.class)).asSubClass(AWTKeyStroke.class);
>
> As I'm not overly familiar with this code and the app context 
> mechanism, in the context of this lint removal exercise, I'd prefer to 
> leave the timing of the checks as they are today.

The problem is what you get is not a Class<AWTKeyStroke> but a Class<? 
extends AWTKeyStroke>
so even if you don't want to use asSubClass, you should at least write:
Class<? extends AWTKeyStroke> clazz = (Class<? extends 
AWTKeyStroke>)AppContext.getAppContext().get(AWTKeyStroke.class);

>
>>
>> and I think the second @SuppressWarnings in getCachedStroke() is
>> unnecessary.
>
> Right you are; I've removed the second @SuppressWarnings in the next 
> iteration of the patch.
>
>>
>>
>> in GraphicsEnvironment, the last line of your diff can be simplified,
>>
>> ge = GraphicsEnvironment.class.cast(geCls.newInstance());
>>
>> can be written
>>
>> ge = (GraphicsEnvironment)geCls.newInstance();
>>
>> which is more readable IMO but will also generate exactly the same
>> bytecode as the current implementation.
>
> Yes, since the type being cast to is unchanging, it is not necessary 
> to do the cast using core reflection; updated in the next iteration.
>
>>
>>
>> in KeyboardFocusManager,
>>
>> private Set<AWTKeyStroke>[] defaultFocusTraversalKeys = new Set[4];
>>
>> should be
>>
>> private Set<AWTKeyStroke>[] defaultFocusTraversalKeys = new Set<?>[4];
>>
>> so @SuppressWarnings("rawtypes") is not needed.
>
> Actually, javac objects to the code you've proposed:
>
> src/share/classes/java/awt/KeyboardFocusManager.java:352: error: 
> incompatible types: Set<?>[] cannot be converted to Set<AWTKeyStroke>[]
>     private Set<AWTKeyStroke>[] defaultFocusTraversalKeys = new 
> Set<?>[4];
>
> (IIRC, I ran into this when I was first putting the changeset together.)

yes, right, you need to reassure the compiler by inserting a cast to 
Set<AWTKeyStroke>[],
   private Set<AWTKeyStroke>[] defaultFocusTraversalKeys = 
(Set<AWTKeyStroke>[])new Set<?>[4];
you still don't need a raw cast here.

raw cast should only be used when you interact with legacy code or 
either Object.getClass or Foo.class.
For the two later it's because the Java spec stupidly introduces raw 
types in the type checking rules.

>
>>
>>
>> in DragGestureEvent, the newEvents should be a List<? extends 
>> InputEvent>,
>> and @SuppressWarnings("rawtypes") should be a 
>> @SuppressWarnings("unchecked")
>
> For the purposes of this clean up effort, I'm not eager to take on 
> more extensive updates to DragGestureEvent than adding the one 
> @SuppressWarnings. I would encourage the awt team to consider the 
> update you've suggested.

I disagree, introducing a raw type where it's no necessary is not a good 
practice.
I don't see the point to add an @SuppressWarnings where you don't need one.
For me, it's a similar change to the one done below.

>
>>
>> in RenderableImageOp,
>> getRenderableSources() should return a Vector of RenderableImage,
>>
>>        public Vector<RenderableImage> getSources() {
>>           getRenderableSources();
>>        }
>>           private Vector<RenderableImage> getRenderableSources() {
>>           Vector<RenderableImage sources = null;
>
> Petr made the same observation and I've addressed that in the second 
> iteration of the patch:
>
>     http://cr.openjdk.java.net/~darcy/8039109.2/

ok, cool.

>
>>
>> all other modifications are OK for me.
>
> Newest iteration at:
>
>     http://cr.openjdk.java.net/~darcy/8039109.3/
>
> Thanks for the review,
>
> -Joe
>

regards,
Rémi