You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Da...@REQUISITE.com on 2003/10/04 01:37:12 UTC
Bug and patch for NodeCachingLinkedList
Greetings,
While working with the NodeCachingLinkedList, I seem to have found a bug in
the implementation of addLast(). Basically, it has to do with the fact that
CommonsLinkedList.createNode() has the parameters named "next", "previous",
"element", but the constructor for Node() names the parameters "previous",
"next", "element". In addNodeBefore(), it passes the parameters in the
order "previous", "next", "element". This works most of the time because of
the way createNode passes the paramters to the Node constructor. However
NodeCachingLinkedList gets them backwards when it populates the cached node.
The following patch fixes the probably temporarily, but I suspect the real
fix is to make sure that createNode and the Node constructor and all calling
methods are consistent:
diff -w -u -r1.7 NodeCachingLinkedList.java
--- NodeCachingLinkedList.java 31 Aug 2003 17:26:44 -0000 1.7
+++ NodeCachingLinkedList.java 3 Oct 2003 23:30:39 -0000
@@ -212,8 +212,8 @@
if (cachedNode == null) {
return super.createNode(next, previous, element);
} else {
- cachedNode.next = next;
- cachedNode.previous = previous;
+ cachedNode.next = previous;
+ cachedNode.previous = next;
cachedNode.element = element;
return cachedNode;
}
Here's a test case that exposes the problem:
public void testRemoveLast()
{
NodeCachingLinkedList list = new NodeCachingLinkedList();
list.addAll( Arrays.asList( new String[]{"value1", "value2"}));
assertEquals( "value1", list.removeFirst() );
list.addLast( "value3");
assertEquals( "value2", list.removeFirst() );
assertEquals( "value3", list.removeFirst() );
list.addLast( "value4" );
assertEquals( "value4", list.removeFirst() );
}
What happens is that the final "removeFirst" throws a NullPointerException
without the patch above.
Let me know if anyone has questions!
David
CONFIDENTIALITY NOTICE This electronic mail transmission and any
accompanying documents contain information belonging to the sender
("Information") that may be confidential and legally privileged. If you are
not the intended recipient, any disclosure, copying,distribution or action
taken in reliance on the Information is strictly prohibited. If you have
received the Information in error,please contact the sender by reply email
and destroy all copies of the original email. Thank you. ²
Re: Bug and patch for NodeCachingLinkedList
Posted by Phil Steitz <ph...@steitz.com>.
Should be fixed now in CVS, in nighlties starting with 05-0ct.
Pls repost or open a Bugzilla ticket at
http://issues.apache.org/bugzilla/enter_bug.cgi?product=Commons
if the fix does not work for you or if you have other problems.
Thanks again for reporting this.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: Bug and patch for NodeCachingLinkedList
Posted by Phil Steitz <ph...@steitz.com>.
David.Hay@REQUISITE.com wrote:
> Greetings,
>
> While working with the NodeCachingLinkedList, I seem to have found a bug in
> the implementation of addLast(). Basically, it has to do with the fact that
> CommonsLinkedList.createNode() has the parameters named "next", "previous",
> "element", but the constructor for Node() names the parameters "previous",
> "next", "element". In addNodeBefore(), it passes the parameters in the
> order "previous", "next", "element". This works most of the time because of
> the way createNode passes the paramters to the Node constructor. However
> NodeCachingLinkedList gets them backwards when it populates the cached node.
>
> The following patch fixes the probably temporarily, but I suspect the real
> fix is to make sure that createNode and the Node constructor and all calling
> methods are consistent:
>
> diff -w -u -r1.7 NodeCachingLinkedList.java
> --- NodeCachingLinkedList.java 31 Aug 2003 17:26:44 -0000 1.7
> +++ NodeCachingLinkedList.java 3 Oct 2003 23:30:39 -0000
> @@ -212,8 +212,8 @@
> if (cachedNode == null) {
> return super.createNode(next, previous, element);
> } else {
> - cachedNode.next = next;
> - cachedNode.previous = previous;
> + cachedNode.next = previous;
> + cachedNode.previous = next;
> cachedNode.element = element;
> return cachedNode;
> }
>
> Here's a test case that exposes the problem:
>
> public void testRemoveLast()
> {
> NodeCachingLinkedList list = new NodeCachingLinkedList();
> list.addAll( Arrays.asList( new String[]{"value1", "value2"}));
> assertEquals( "value1", list.removeFirst() );
> list.addLast( "value3");
> assertEquals( "value2", list.removeFirst() );
> assertEquals( "value3", list.removeFirst() );
> list.addLast( "value4" );
> assertEquals( "value4", list.removeFirst() );
> }
>
> What happens is that the final "removeFirst" throws a NullPointerException
> without the patch above.
>
> Let me know if anyone has questions!
Thanks! This is definitely a problem, which should be addressed by
making the parameter order consistent, as you describe. Thanks for
pointing this out.
Phil
>
> David
> CONFIDENTIALITY NOTICE This electronic mail transmission and any
> accompanying documents contain information belonging to the sender
> ("Information") that may be confidential and legally privileged. If you are
> not the intended recipient, any disclosure, copying,distribution or action
> taken in reliance on the Information is strictly prohibited. If you have
> received the Information in error,please contact the sender by reply email
> and destroy all copies of the original email. Thank you. ²
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org