harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aleksey Shipilev" <aleksey.shipi...@gmail.com>
Subject Re: [classlib][luni] java/util/HashMap.java bug waiting to happen?
Date Wed, 21 May 2008 05:05:21 GMT
Hi, I would rather go (3) way as inlining would not exhibit additional
overhead, but (2) would.

Thanks,
Aleksey.

On Wed, May 21, 2008 at 6:19 AM, Jim Yu <junjie0122@gmail.com> wrote:
> Hi Oli,
>
> I agree with you. (2) will be simple and can prevent potential risk.
>
> 2008/5/21 Oliver Deakin <oliver.deakin@googlemail.com>:
>
>> Hi Mark,
>>
>> Agreed that this could cause problems in the future if it is called from
>> another method expecting it to behave properly for entries that do not exist
>> in the HashMap. Three options immediately spring to mind:
>> 1) Leave it as is - we won't hit the non-existent entry problem at the
>> moment due to the method caller always ensuring the entry exists.
>> 2) Add a condition after the while loop to return without altering the
>> HashMap if m.next is null.
>> 3) Get rid of the removeEntry() method by inlining it into the calling
>> method and fixing it up to handle non-existent entries.
>>
>> I think (2) is a simple fix, although (3) is also reasonable since there is
>> only a single call to this method. I think (1) is, as you say, a problem
>> waiting to happen.
>> Personally I'd go with (2).
>>
>> Regards,
>> Oliver
>>
>>
>> Mark Hindess wrote:
>>
>>> While reviewing Aleksey's improvements in HARMONY-5791, I noticed the
>>> following lines in HashMap.java (line 682):
>>>
>>>    final void removeEntry(Entry<K, V> entry) {
>>>        int index = entry.origKeyHash & (elementData.length - 1);
>>>        Entry<K, V> m = elementData[index];
>>>        if (m == entry) {
>>>            elementData[index] = entry.next;
>>>        } else {
>>> [1]         while (m.next != entry && m.next != null) {
>>>                m = m.next;
>>>            }
>>> [2]         m.next = entry.next;
>>>
>>>        }
>>>        modCount++;
>>>        elementCount--;
>>>    }
>>>
>>> The while loop at [1] has two conditions.  The first relates to finding
>>> the entry and the second relates to getting to the end of the linked
>>> list (i.e. not finding the entry).  Executing the line [2] only really
>>> makes sense if the first condition was met; if the second was met then
>>> it might corrupt the list.
>>>
>>> As it happens, the only caller of this method in HashMap.java does
>>> ensure that the entry is present in the HashMap and thus in the linked
>>> list but it still seems like a problem waiting to happen (or at the very
>>> least a redundant condition on a while loop).
>>>
>>> It isn't broken so I'm not sure what (if anything) to "fix".
>>>
>>> Regards,
>>> -Mark.
>>>
>>>
>>>
>>>
>>>
>>
>> --
>> Oliver Deakin
>> Unless stated otherwise above:
>> IBM United Kingdom Limited - Registered in England and Wales with number
>> 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
>> PO6 3AU
>>
>>
>
>
> --
> Best Regards,
> Jim, Jun Jie Yu
>
> China Software Development Lab, IBM
>

Mime
View raw message