The revised version looks good to me. It's worth getting all four. On Feb 23 2012, at 06:46 , Seán Coffey wrote:
> ok, > > I've updated the changes (and bug synopsis) based on comments. > equals method tweaked for : > > Collections.SynchronizedList > Collections.SynchronizedSet > Collections.SynchronizedMap > Collections.UnmodifiableEntry > > Testcase updated also. JCK java_util tests run also. > > http://cr.openjdk.java.net/~coffeys/webrev.7144488.1/ > > regards, > Sean. > > On 23/02/2012 09:56, David Holmes wrote: >> On 23/02/2012 7:55 PM, Alan Bateman wrote: >>> On 23/02/2012 02:44, David Holmes wrote: >>>> >>>> All of the SynchronizedX.equals methods should be fixed ie >>>> SynchronizedSet, SynchronizedList and SynchronizedMap. It should >>>> always be valid to ask if a.equals(a). The idiomatic form used >>>> elsewhere (CheckedXXX) is: >>>> >>>> public boolean equals(Object o) { return o == this || list.equals(o); } >>>> >>>> I'm not a fan of collections containing themselves, but I think it is >>>> simple to fix contains(o)/contains[Key]|[Value](o) and remove(o) in a >>>> similar way. Though none of the wrapped collections currently handle >>>> that case, so I'm okay with not addressing this part. >>>> >>>> I don't see the recursion potential in hashCode() in the wrappers. >>> The hashCode of a Set requires summing the hash codes of each of the >>> elements so it can't work for the case that a set contains itself. >> >> Right, but that's a general problem with collections, not with the wrappers >> specifically. >> >> Cheers, >> David >> >>> Anyway, I think the best we can do it to have the equals methods use the >>> above form. I wasn't aware of the warning in List that Sean pointed out >>> and maybe there should be similar warnings elsewhere. >>> >>> -Alan
