<Swing Dev> JDK 9 RFR of JDK-8043550 : Fix raw and unchecked lint warnings in javax.swing.*
Petr Pchelko
petr.pchelko at oracle.com
Thu Jul 3 15:30:56 UTC 2014
Hello, Joe.
The fix looks good.
>>> (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.
Ok, I was thinking about the same. Could you please file a P4 bug against swing? We will handle it further. Thank you.
With best regards. Petr.
> On Jul 3, 2014, at 7:22 PM, Joe Darcy <joe.darcy at oracle.com> wrote:
>
> 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 <joe.darcy at oracle.com> 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 <joe.darcy at oracle.com> 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
>
More information about the swing-dev
mailing list