Hi,
Cleary i should be more careful reviewing while eating my breakfast.
> On 10 Jan 2017, at 09:00, Claes Redestad <claes.redes...@oracle.com> wrote:
>
> Hi Paul,
>
> On 2017-01-10 17:32, Paul Sandoz wrote:
>> Hi,
>>
>> ImmutableCollections
>> —
>>
>> Did you forget or intentionally omit overriding the hashCode for List2 and
>> ListN?
>
> Intentionally omitted: since List2 and ListN have O(1) get performance,
> the Iterator provided by AbstractList seems good enough for now.
>
Ok. Given the collections are small i wondered if the iterator allocation was
significant.
>>
>>
>> 481 @Override
>> 482 public int hashCode() {
>> 483 int h = 0;
>> 484 for (E e : elements) {
>> 485 if (e != null) {
>> 486 h += e.hashCode();
>> 487 }
>> 488 }
>> 489 return h;
>> 490 }
>>
>> No need for the null check of e.
>
> Actually null checks are needed: SetN elements is a sparse array with
> null at indexes where there is no actual entry.
>
Doh!
> Or are you suggesting we do the following?
>
No, i forgot the table can contain null values.
> for (E e : elements) {
> h += Objects.hashCode(e);
> }
>
>> If you override hashCode should you override equals as well?
>
> I need to look into this one and get back to you, might be
> unintentionally left out.
>
Ok.
>>
>>
>> 617 @Override
>> 618 public int hashCode() {
>> 619 return k0.hashCode() ^ v0.hashCode();
>> 620 }
>>
>> That does not conform to the specification of Map.hashCode:
>>
>> /**
>> * Returns the hash code value for this map. The hash code of a map is
>> * defined to be the sum of the hash codes of each entry in the map's
>> * {@code entrySet()} view. This ensures that {@code m1.equals(m2)}
>>
>> (Your tests might be lucky with the summing of hash codes with distinct
>> ranges of bits set?)
>
> I think you're mistaken: k0 and v0 constitute the sole entry of Map1,
> and k0.hashCode() ^ v0.hashCode() is the hash code for said entry;
> compare HashMap.Node.hashCode().
>
Another Doh! I got crosseyed thinking this for a map holding two entries.
Paul.
>>
>>
>> 669 @Override
>> 670 public boolean containsValue(Object o) {
>> 671 for (int i = 1; i < table.length; i += 2) {
>> 672 Object v = table[i];
>> 673 if (v != null && o.equals(v)) {
>> 674 return true;
>> 675 }
>> 676 }
>> 677 return false;
>> 678 }
>>
>> No need for the null check for v, can pass table[i] directly to o.equals
>
> Again, table is sparse and contain null entries when there's no map
> entry at that index in the table. In this case o.equals(null) would
> likely return the right thing (false) for all objects, but seems a bit
> odd to me.
>
> Thanks!
>
> /Claes
>
>>
>> Paul.
>>
>>> On 10 Jan 2017, at 03:50, Claes Redestad <claes.redes...@oracle.com> wrote:
>>>
>>> Hi,
>>>
>>> please review this change to improve startup/warmup characteristics
>>> and performance of the immutable collection factories:
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166365
>>> Webrev: http://cr.openjdk.java.net/~redestad/8166365/webrev.01/
>>>
>>> While filed as an RFE, and a more thorough examination of the hierarchy
>>> of the ImmutableCollection classes has been discussed[1], this more
>>> limited patch resolves some immediate issues related to module system
>>> bootstrap and is therefore motivated to push to 9.
>>>
>>> Thanks!
>>>
>>> /Claes
>>>
>>> [1] we should likely not inherit Iterator classes that check for
>>> co-modification etc,
>>
