[Mono-dev] BitVector32 patch

[Rodrigo Kumpera]

On Tue, Sep 9, 2008 at 8:09 PM, Scott Peterson <lunchtimemama at gmail.com>wrote:

> Sorry about the mixed objectives. I figured it was a short enough
> patch that it would be easier to review all at once, rather than
> sending two or three little patches to the list. Thanks for reviewing
> it anyway, Rodrigo.
>

The issue is not just reviewing, but reducing the amount of code changes to
make
it easier to trace them back. This is specially important for long term
maintenance.



>
> >> 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?
> >
>
> The value of mask will always be one less than a power of two (see
> CreateSection for the implementation). So it could be 1 or 3 or 7, but
> not 2 or 4. This means the hash will be unique for any two mask and
> offset values.
>


You're right. It won't introduce collisions. Let's use your version then.



> >> @@ -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.
> >
>
> This is just a habit of mine. I might have put it in by mistake while
> doing some other change which I got rid of. I can whack it.


Since this change doesn't make any kind of difference, neither for
readability (formating)
or runtime (performance/correctness) we better avoid it.


Otherwise, please commit the changes we talked about here.

Thanks,
Rodrigo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20080909/98b7c410/attachment.html