harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oliver Deakin <oliver.dea...@googlemail.com>
Subject Re: [classlib][luni] java/util/HashMap.java bug waiting to happen?
Date Tue, 20 May 2008 19:28:24 GMT
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


Mime
View raw message