[Mono-devel-list] [PATCH] Additional System.Collections.Generic.{Dictionary, EqualityComparer} f

[Raja R Harinath]

Hi,

> >>> David Waite <dwaite at gmail.com> 06/18/05 12:19 PM >>>
> 2005-06-16  David Waite  <mass at akuma.org>
>
>     * Dictionary.cs (EnumerationMode): Remove.
>     (Enumerator): Remove return type flag - legacy return is expected
to
>     always return a DictionaryEntry

Neat.

>     (Enumerator.MoveNext): Fix endless loop

???  I couldn't find this fix.  The patch to this function was:

                        public bool MoveNext ()
                        {
+                               if (_dictionaryTable == null)
+                               {
+                                       throw new
ObjectDisposedException(null);
+                               }
+
+                               // if we are on an entry, try to move to
the next linked entry
+                               // (in the bucket)
                                if (_current != null)
+                               {
                                        _current = _current.Next;
-
+                               }
+                               // if we are not on an entry, assume we
are either finished
+                               // with a bucket, or not currently on
the first
bucket. walk
+                               // the table searching for a non-null
entry
                                while (_current == null && _nextIndex <
_dictionaryTable.Length)
                                        _current = _dictionaryTable
[_nextIndex++];

Apart from the "object disposed", It just adds a couple of braces (see
below)
and comments.  Anyway, I'm pretty sure there's no infloop in there --
the loop
is either never entered, or it always makes progress towards ending the
loop,
either by setting _current to non-null, or by stepping past the end of
the array.

BTW, can you fix your coding style to match the style in
mcs/class/README?  In particular, the '{' comes on the same line as
the 'if', and we avoid creating a statement block if it contains only
one statement.  Also, there must be a space before the '(' of a
function invocation.

The comments above are too IMO too verbose -- and restate what
the code says.  So, this patch should essentially have just added the
following lines:

  if (_dictionaryTable == null)
	throw new ObjectDisposedException (null);

  // Pre-condition: _current == null => this is the first call to
MoveNext ()

I wouldn't mind the following additional comment, though it's somewhat
redundant, just for the symmetry:

  // Post-condition: _current == null => this is the last call to
MoveNext ()

(pedantically, it should read "last allowable call", but that makes the
comment
less funny[1] ;-)

                                public void Dispose ()
                                {
-                                       _hostEnumerator = null;
+                                       _hostEnumerator =
default(Dictionary<TKey,TValue>.Enumerator);
                                }

Can't this just be _hostEnumerator.Dispose(), too?

Otherwise, your patch looks good.  The unit tests are particularly
welcome.
Can you re-submit with the coding-style fixes?

Thanks,
- Hari

[1] Yeah, I have a strange sense of humour.