Re: [jbosscache-dev] New collections

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. There is now a Immutables factory class that does copying, conversion, and wrapping of list, set, collection, and map. So as long as everything uses this, it should be easy to swap out implementations. As suggested, I went ahead and left ImmutableListCopy, however the other types use a wrapped clone/copy. Since the marshalling code needs to detect these types, I ended up having to reimplement the Collections.unmodifiableXXX types (not a lot of code though). These changes saved another copy in the unmarshalling code, since it needs to construct a new HashMap anyway. In this scenario, the Immutables.immutableMapWrap() is used, which does not copy. Also the duplicate copy in CacheInvocationDelegate.getChildrenNames was also removed using the set variant of the wrap method. -- Jason T. Greene JBoss, a division of Red Hat