Jason T. Greene wrote:
>> Ok, today I went ahead and made these changed (commit 6536).
So in
>> case anything breaks, you know why :) I did run through the unit
>> tests locally though, with no failures.
>
> I think it did break a few things - see the tests in
> o.j.c.notifications. :-)
Ok, I'll take a look at those. I remember them passing for me before.
This is fixed now. I never saw the failure because the failing test
because I was executing the wrong test profile. I thought it finished a
little too early :)
> Also, looking at Immutables.immutableMapCopy(), isn't it
inefficient
> that you attempt to call clone() using reflection, catch exceptions on
> failure, then try a copy constructor, catch exception on failure, and
> finally attempt a new HashMap()? Given that this would most likely be
> used with either a FastCopyHashMap or a HashMap, shouldn't we just
> check for instanceof of these types and use the copy constructor
> directly, especially given that there is hardly any performance diff
> between FCHM.clone() and it's copy constructor?
Yeah I was originally planning on doing a instanceof check on known
types, I just forgot to commit that update. It is using clone though.
This is done now. It uses clone because the copy-ctor of almost all the
maps increases the size of the table.
> Cloneable is such an arse interface - they really should have
exposed
> clone() as a public method there. Solves a lot of problems, no need
> for reflection, etc.
Yeah I agree it's a screwed up design.
The other day I was thinking this would be so easy to fix in the JDK.
Add a new interface called ReallyCloneable that extends Cloneable and
requires the clone method. Then introduce a System.clone(Object object)
which is smart enough to try ReallyCloneble first, and then barring that
use reflection.
--
Jason T. Greene
JBoss, a division of Red Hat