I will need to spend more time reviewing the HashMap/LinkedHashMap
implementations but here are some initial comments.
Mike
HashMap::
- The "Sorry if you don't like..." comment can be omitted.
SpliteratorCharacteristics::
- style comment : I always define my assertXXX in the (value, expected,
message) form as well as the (value, expected). Being able to pass the test
case name for message is often useful context. It is a little more boilerplate.
- void assertNullComparator(Collection<?> c) {
210 assertNullComparator(c.spliterator());
211 }
intentionally throws NPE if c is null? Why not instead assertNull or
assertNonNull?
MapBinToFromTreeTest::
- thank you for using "hg move" to preserve history. :-)
- I try to encourage @DataProvider methods to return Iterator<Object[]> rather
than Object[][]. I haven't made an incremental data provider which generates
cases in next() yet.... but I could someday.
- getCollector(), put(), remove() could all be static
- put(SIZE, m, (i, s) -> { }); could be put(SIZE, m, (i, s) -> { assertEquals(i
+ 1,s); }); or some other check in put that the construction was expected.
On Aug 21 2013, at 05:25 , Paul Sandoz wrote:
> Hi,
>
> Here are Doug's Linked/HashMap changes, discussed in a previous thread, as a
> webrev:
>
>
> http://cr.openjdk.java.net/~psandoz/tl/JDK-8023463-Linked-HashMap-bin-and-tree/webrev/
>
> I also added some tests related to characteristics associated with fixing
> another bug.
>
> Looking at the diffs will be tricky given there are so many changes.
>
>
> I fixed unchecked warnings in LinkedHashMap, but have not done so for
> HashMap, where there are many more casts from Node to TreeNode. One way to
> solve that is with a static method:
>
> @SuppressWarnings("unchecked")
> static <K, V> TreeNode<K, V> asTreeNode(Node<K, V> n) {
> return (TreeNode<K, V>) n;
> }
>
> but i dunno what the performance implications are. Perhaps it's best to
> simply stuff @SuppressWarnings on methods of TreeNode rather than trying to
> modify code just for the sake of reducing the scope.
Always a tough spot. I've favoured adding annotations to variables wherever
possible but when it becomes necessary to suppress for entire methods it does
feel like shot
The alternative approach of using a method would almost certainly be inlined
and as a result would have little (if any) impact on performance.
I still lean towards suppressing on variables and then see what's left.
>
> A JPRT job has been kicked off.
>
> I recommend when this code goes in we look closely at code coverage results
> to see if we are missing areas testing tree functionality and update/add new
> tests accordingly.
>
> Paul.
>
