Patch: Re: LinkedList.clone() bug

Bryce McKinlay bryce@waitaki.otago.ac.nz
Mon Jul 16 18:56:00 GMT 2001


Brad Fitzpatrick wrote:

> Without testing, my guess is that won't work, as it was seeming like right
> after the try/catch, (copy == this) was true.

My guess is that this was just confused output from the debugger (did you
build libgcj without optimization?).

> To me, it would seem like clone() would throw an NullPointerException when
> it did copy.size = 0, since copy should still be null?  Wouldn't
> super.clone() throw an exception, since AbstractSequentialList has no
> clone()?

The super.clone() call is actually invoking clone() from java.lang.Object.
Object.clone() is designed to be callable from subclasses which implement
Cloneable. Since we know LinkedList implements Cloneable, the
CloneNotSupportedException can never happen.

> But in my testcase, both l1 and the cloned l2 had their contents doubled,
> which again suggests this == copy.
>
> Maybe I don't understand what's going on (quite likely), but it seems as if
> the LinkedList clone() is not quite right.

You're right, the method was buggy. The result from the call to Object.clone()
is an exact memory copy of the source object. The cloned object is different
from the original object (ie this != copy) but all its fields contain the same
values. The problem was that the method was naively trying to clear the copy
object by setting its size to 0. This technique works when using the regular
add() because it checks for the size == 0 case explicitly and resets the
"first" and "last" fields to point to the only Entry object. However, the
addAll() method is more efficient and does not have such a check. It sees that
"last" is non-null and proceeds to add entries immediately following this in
the list. These new entries are now visible in both lists because they share
the same Entry objects (and the structure of both lists is now corrupted).

I'm checking in the fix:

2001-07-17  Bryce McKinlay  <bryce@waitaki.otago.ac.nz>

 * java/util/LinkedList.java (clone): Clear the copy list with clear(),
 not by setting its size field.

regards

  [ bryce ]



More information about the Java mailing list