[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] JDK 9 RFR of JDK-8039109 : Fix unchecked and raw lint warnings in java.awt
From:       Joe Darcy <joe.darcy () oracle ! com>
Date:       2014-05-05 23:00:39
Message-ID: 53681817.7050804 () oracle ! com
[Download RAW message or body]

Hi Phil,

On 5/5/2014 3:51 PM, Phil Race wrote:
> I added 2-dev to have this recorded there since 13/20 files  in the 
> webrev are 2D.
>
> I didn't see any thing that's a definite problem although I don't
> see what difference the following makes. How does it suppress a warning ?
>
>  Collections.EMPTY_SET  = > Collections.emptySet()

Collections.emptySet() is a generic method and when it is used, javac 
can infer a site-specific value of the type parameter, inferring a 
Set<String> instead of a Set<Integer>, etc. In contrast, the field 
Collections.EMPTY_SET only has a single type. However, calling 
Collections.emptySet() still uses a single shared empty set instance 
across types.

>
> I expect see you added the tmp var here as somewhere to hang the 
> annotation
> as I presume it must be on a declaration .. unfortunately a bit yucky 
> tho'
>
>  262             @SuppressWarnings("unchecked")
>  263             Vector<TrayIcon> tmp = 
> (Vector<TrayIcon>)AppContext.getAppContext().get(TrayIcon.class);
>  264             icons = tmp;

For this sort of change, annotation must be applied to declarations. If 
there is not a local variable, the condition for the entire method has 
to be suppressed, which is usually a worse trade-off.

Thanks,

-Joe

>
> -phil.
>
> On 5/5/2014 3:22 PM, Jim Graham wrote:
>> I took a look at the j.a.image and j.a.geom files and everything 
>> looked fine.
>>
>>             ...jim
>>
>> On 5/4/14 9:44 AM, Joe Darcy wrote:
>>> Hello,
>>>
>>> Any further comments on the latest version
>>>
>>>       http://cr.openjdk.java.net/~darcy/8039109.3/
>>>
>>> Thanks,
>>>
>>> -Joe
>>>
>>> On 4/30/2014 4:04 PM, 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.
>>>>
>>>>>
>>>>> 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.)
>>>>
>>>>>
>>>>>
>>>>> 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.
>>>>
>>>>>
>>>>> 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/
>>>>
>>>>>
>>>>> all other modifications are OK for me.
>>>>
>>>> Newest iteration at:
>>>>
>>>>     http://cr.openjdk.java.net/~darcy/8039109.3/
>>>>
>>>> Thanks for the review,
>>>>
>>>> -Joe
>>>>
>>>
>

[prev in list] [next in list] [prev in thread] [next in thread]