On Feb 25 2013, at 01:51 , Alan Bateman wrote: > On 25/02/2013 04:06, Mike Duigou wrote: >> I have created an issue for fixing the introduced regression and created a >> regression test that would have caught the error (fix a bug, write a >> test...). >> >> 8008785: IdentityHashMap.values().toArray(V[]) broken by JDK-8008167 >> >> http://cr.openjdk.java.net/~mduigou/JDK-8008785/0/webrev >> >> Mike > The fix to IdentityHashMap looks good. It is surprising that it wasn't caught > by tests in the jdk repository so a reminder that we always need to check > that we have good test coverage when doing performance fixes.
Yes, I should have checked not just that the existing tests passed but that there was a specific test for what was being changed. I was very surprised to discover after the fact that there was no test at all for ToArray. > > The webrev suggests you've replaced Map/Collisions.java and assume that is > not the intention. Otherwise the new test looks okay, an alternative would > have been to expand MOAT. Webrev doesn't properly recognize 'hg copy' and sees it as a rename. I started ToArray.java by copying Collisions.java Mike > -Alan.
