Hi Petr,
On 07/01/2014 01:51 PM, Petr Pchelko wrote:
Hello, Joe.
(Is is correct that this code is using "==" rather than ".equals" to compare
strings? If .equals were correct, I would change this method to use strings in switch.)
Interesting question. The history of this code is lost 10 years ago, so I don’t
know why == is used here instead of equals.
Just looking at the code I do not see any real reason to use ==, so I would say
it’s a bug. So I would say we can replace it with switch.
Upon further reflection, I'd prefer the change to using a .equals based
comparison being done under a different changeset. Doing so will make it
easier to isolate any problems which may arise strictly from
generification and warning removal.
(This degree of explanation struck me as overkill for a comment, but I can work
on adding something along these lines if you think it would be helpful to
future readers of the code.)
Thank you for the detailed explanation, could you please make the comment
slightly more explanatory? I think that adding the info that Number subclasses
are not comparable with each other as Numbers would be enough.
Comment expanded.
The changes in javax.swing.tree and javax.swing.undo are new in this request.
I’ve looked at these too, no new comments about them.
Update webrev:
http://cr.openjdk.java.net/~darcy/8043550.1/
Thanks,
-Joe
Thank you.
With best regards. Petr.
On Jul 1, 2014, at 8:22 PM, Joe Darcy <[email protected]> wrote:
Hi Petr,
On 07/01/2014 01:17 AM, Petr Pchelko wrote:
Hello, Joe.
It's hard to understand what to review and what not to review. Could you please
generate a separate webrev for the changes that needs to be reviewed next time?
I've looked only to the javax.swing package, so I'm not sure I've reviewed
everything that needed to be reviewed.
1. JComboBox: 1983 - no need to suppress warning
Unnecessary suppression removed.
2. JComponent: 4121 - the change is incorrect. Value can be not an instance of
Set. It depends on the property name. See line 4124 where it's cast to Boolean
Noted; I've changed this to have a declaration of strokeSet in each of the if
block where the unchecked cast used to occur.
(Is is correct that this code is using "==" rather than ".equals" to compare
strings? If .equals were correct, I would change this method to use strings in switch.)
3. SpinnerNumberModel: 90 - There's a typo in the comment with braces. Also, could you
please clarify what do you mean under "awkward to use"?
Typo fixed.
The actual Comparable mins and max people are likely to pass in are values of
type java.lang.Double, java.lang.Integer, etc. Such types are correctly
declared as Comparable to themselves:
public final class Double extends Number implements Comparable<Double> {...}
public final class Integer extends Number implements Comparable<Integer>
{...}
This means that in the class for Double, there are two compareTo methods, one
that takes a Double argument and one that takes an Object argument; the latter
is the bridge method for the former. Of note is that there is *not* a compareTo
method taking a Number as an argument.
The Number type is a bit misnamed as an abstraction; it just means "convertible to
primitive." Therefore, two arbitrary Number types are not mutually Comparable so it
is sensible that Double doesn't have a compareTo bridge method for Number.
If the minimum and maximum values were declared as Comparable<? extends Number> with
the aim of comparing two Number instances with them, in practice that would mean calling a
compareTo(Number) method which doesn't exist. In that sense, it would be awkward to use
Comparable<? extends Number> ;-)
(This degree of explanation struck me as overkill for a comment, but I can work
on adding something along these lines if you think it would be helpful to
future readers of the code.)
The changes in javax.swing.tree and javax.swing.undo are new in this request.
Thanks,
-Joe
With best regards. Petr.
On 30 июня 2014 г., at 20:20, Joe Darcy <[email protected]> wrote:
Hello,
Please review the final batch of changes to resolve all the unchecked and
rawtypes warnings in swing:
JDK-8043550 : Fix raw and unchecked lint warnings in javax.swing.*
http://cr.openjdk.java.net/~darcy/8043550.0/
For completeness, this webrev also includes the in-progress changes for
8043548: Fix raw and unchecked lint warnings in javax.swing.plaf.*
8042849: Fix raw and unchecked warnings in com.sun.java.swing
For 8043548, there is still some review work going on to sort out some of the
details; 8042849 has been approved, but not pushed yet as it could conceivably
need updating based on other changes in swing.
It was not immediately clear how javax.swing.tree.TreeNode.children(), which
returns a raw Enumeration, should be generified. I changed it to
Enumeration<TreeNode> children();
and that seems to work fine. Something like
Enumeration<? extends TreeNode>
with a covariant override would save a few casts in a private implementation,
but generally doesn't seem to be a good trade-off since many normal clients
could be inconvenienced dealing with the wildcard.
How to best generify the field javax.swing.KeyboardManager.containerMap was not
immediately clear either. The documentation implies the nested Hashtable could be
something like a Hashtable<KeyStroke, Object>
68 /**
69 * maps top-level containers to a sub-hashtable full of keystrokes
70 */
71 Hashtable<Container, Hashtable<Object, Object>> containerMap = new
Hashtable<>();
but that doesn't work out throughout the entire file since there is code that add
entries using a Class object as the key. In the end, I just used the not-very-precise
Hashtable<Object, Object>.
Thanks,
-Joe
