Re: [jbosscache-dev] New collections

On 7 Aug 2008, at 05:35, Jason T. Greene wrote: > Manik Surtani wrote: >> On 5 Aug 2008, at 14:42, Jason T. Greene wrote: >>>>> >>>>> What I would like to do, provided everyone is ok with this, is >>>>> switch this code to static factory methods like so: >>>>> >>>>> Immutables.immutableCopy(Map map); >>>>> Immutables.immutableCopy(List list); >>>>> Immutables.immutableCopy(Set set); >>>>> >>>>> These will then just call clone and Collections.unmodifiableXXX as >>>>> appropriate. We can then add optimizations later if they become >>>>> available. >>>> Would these be more generic, to suit any collection >>>> implementation? Remember that not all Maps, for example, implement >>>> Cloneable or have a copy constructor. And wrapping any Map passed >>>> in as a FastCopyHashMap may break correctness, for example, if a >>>> SortedMap were passed in. >>> >>> Right, they would use instanceof to check all the known map types as >>> a fast-path optimization. After that it would check for cloneable, >>> and use reflection to invoke clone. It would then look for a copy >>> constructor, and failing that it would then create a different >>> collection type. We could for example, in the case of a non-copyable >>> SortedMap, use a TreeMap, and order is maintained. >> Right yes, you would need the logic in place to do this. >> -- > > 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. :-)

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?

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.