[Mono-dev] BitVector32 patch

[Rodrigo Kumpera]

Hi Scott,

Mixing different kinds of changes together in the same patch makes the
reviewing process hard.
And even if the patch was ok, the mantainer would have to split it anyway to
make easier in the future
to track down changes.

If you could, please, split up your patches it would be nice.

About your patch, I left some comments  inline.

Thanks,
Rodrigo



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

> @@ -84,14 +84,12 @@
>
>              public override int GetHashCode ()
>              {
> -                return (((Int16) mask).GetHashCode () << 16) +
> -                       ((Int16) offset).GetHashCode ();
> +                return mask << offset;
>              }
>
>

What is the possible range of offset? Can't it be bigger than 16 and discard
even more bits
of entropy from mask?



>
> @@ -184,9 +184,8 @@
>              if (maxValue < 1)
>                  throw new ArgumentException ("maxValue");
>
> -            int bit = HighestSetBit(maxValue) + 1;
> -            int mask = (1 << bit) - 1;
> -            int offset = previous.Offset + NumberOfSetBits
> (previous.Mask);
> +            int mask = (1 << HighestSetBit(maxValue)) - 1;
> +            int offset = previous.Offset + HighestSetBit(previous.Mask);
>
>              if (offset > 32) {
>                  throw new ArgumentException ("Sections cannot exceed 32
> bits in total");
>

Shouldn't it be "int mask = (1 << (HighestSetBit(maxValue) + 1)) - 1;"?

Besides that, I think the bit searching function should use a binary search
which has better
performance.


Cheers,
Rodrigo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20080806/e9ba10ce/attachment.html