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
