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
