[Mono-dev] BitVector32 patch

[Rodrigo Kumpera]

Hi Scott,

Your patch mixes formating with real changes, which is not a nice thing, but
given it's size, I'll review it anyway.

But, please, mixing many diferent kind of changes in the same patch will
only cause frustration for everyone.
For the contributor for having a long review time or, worse, just take a
straight no because on the length.
For reviewers, reading long patches with all sorts of changes takes a lot
longer and makes it harder.

2008/8/26 Scott Peterson <lunchtimemama at gmail.com>

> OK, the bug is fixed and the fix is committed. I've done up another
> patch with the other minor fixes (a few logical things and a few style
> things). Patch is attached and inline.
>
> - Scott
>
> Index: class/System/System.Collections.Specialized/BitVector32.cs
> ===================================================================
> --- class/System/System.Collections.Specialized/BitVector32.cs  (revision
> 111591)
> +++ class/System/System.Collections.Specialized/BitVector32.cs  (working
> copy)
>  @@ -84,14 +82,12 @@
>
>                        public override int GetHashCode ()
>                        {
> -                               return (((Int16) mask).GetHashCode () <<
> 16) +
> -                                      ((Int16) offset).GetHashCode ();
> +                               return mask << offset;
>                        }
>

This change will reduce the entropy of the hash function.
With your change the following sections will have the same hash code: (mask
1, offset 2), (mask 2, offset 1), (mask 4, offset 0).
I believe the equivalent is something like (mask << 16) + offset. What do
you think?



>                         public override string ToString ()
>                        {
> -                               return "Section{0x" +
> Convert.ToString(mask, 16) +
> -                                      ", 0x" + Convert.ToString(offset,
> 16) + "}";
> +                               return ToString (this);
>                        }
>

Good catch, it's ok to commit.


> @@ -197,10 +195,7 @@
>
>                public override bool Equals (object o)
>                {
> -                       if (!(o is BitVector32))
> -                               return false;
> -
> -                       return bits == ((BitVector32) o).bits;
> +                       return (o is BitVector32) && bits == ((BitVector32)
> o).bits;
>                }
>
>                public override int GetHashCode ()
>

This is a nice simplification, it's ok to commit.




>
> @@ -218,7 +213,7 @@
>                         StringBuilder b = new StringBuilder ();
>                        b.Append ("BitVector32{");
>                        long mask = (long) 0x80000000;
> -                       while (mask > 0) {
> +                       while (mask != 0) {
>                                b.Append (((value.bits & mask) == 0) ? '0' :
> '1');
>                                mask >>= 1;
>                        }
>
>
> I don't see the point of this change.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20080909/6ef587c3/attachment-0001.html