[jbosscache-dev] New collections

[Jason T. Greene]

Manik Surtani wrote:
> 
> 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.  :-)

Ok, I'll take a look at those. I remember them passing for me before.

> 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.

> 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.

-- 
Jason T. Greene
JBoss, a division of Red Hat