Skip to content

Commit

Permalink
Fix bug mono#311: On LinkedList.Clear, detach each node instead of dr…
Browse files Browse the repository at this point in the history 
…opping them en masse.
  • Loading branch information
Todd Foster committed Aug 17, 2011
1 parent 57bfe64 commit e787a12
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 3 deletions.
5 changes: 2 additions & 3 deletions mcs/class/System/System.Collections.Generic/LinkedList.cs
View file
Expand Up @@ -182,9 +182,8 @@ public void AddLast (LinkedListNode <T> node)

public void Clear ()
{
count = 0;
first = null;
version++;
while (first != null)
RemoveLast();
}

public bool Contains (T value)
Expand Down
5 changes: 5 additions & 0 deletions mcs/class/System/Test/System.Collections.Generic/LinkedListTest.cs
View file
Expand Up @@ -84,8 +84,13 @@ public void NonCircularNodeTest ()
[Test]
public void ClearTest ()
{
LinkedListNode <int> node = intlist.First;
intlist.Clear ();

Assert.AreEqual (0, intlist.Count);
Assert.AreEqual (2, node.Value);
Assert.IsNull (node.Next);
Assert.IsNull (node.Previous);
}

[Test]
Expand Down

4 comments on commit e787a12

@jstedfast
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason you used RemoveLast() instead of RemoveFirst()?

@jstedfast
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you should be using Remove() instead of either RemoveFirst() or RemoveLast(), since you've already checked that first != null.

@jstedfast
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the most performant solution would be:

while (first != null)
Remove (first.back);

I'm guessing you chose RemoveLast() due to RemoveFirst() incurring an extra assignment per loop than RemoveLast().

@toddfoster
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct: I used RemoveLast to possibly avoid an extra assignment.

I chose to use RemoveLast() instead of Remove(first.back) because it was there (DRY). Let it take care of any housekeeping or optimization, including any future code changes.

Please sign in to comment.