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 <[email protected]> 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 <[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 >
