Make class BitField able to use 32 bits of a uint32....

Make class BitField able to use 32 bits of a uint32.

Although algorithmically correct, the compiler would not allow to instantiate
a BitField that uses all 32 bits without warnings about a too large shift
count. As a consequence we were limited to 31 bit values when using BitField.

This happened when instantiating a bitfield BitField<T, shift, size> with 
[shift=0, size=32] or [shift=31, size=1] or more general any 
[shift=X, size=32-X]

As side-effect of the new implementation the compiler now warns if we ever
try instantiating a bitfield with size 0.


Committed: http://code.google.com/p/v8/source/detail?r=3910

[fschneider, 2010-02-16 19:01:27]

Small code review for you :)

[Kevin Millikin (Chromium), 2010-02-17 10:16:33]

Nice.  I've got some questions:

http://codereview.chromium.org/606063/diff/3002/2004
File src/codegen.h (right):

http://codereview.chromium.org/606063/diff/3002/2004#newcode521
src/codegen.h:521: // Minor key encoding in 32 bits AAAAAAAAAAAAAAAAAAAAAFI
A(rgs)F(lag)I(nloop).
I think these comments should go.  It's impossible to count the A's, and the bit
field declarations are self-documenting and authoritative (and nobody cares
about the specific bit positions if they use the accessors anyway).

I think a simple

// Minor key is encoded in 32 bits.

is better.

http://codereview.chromium.org/606063/diff/3002/2004#newcode522
src/codegen.h:522: class InLoopBits: public BitField<InLoopFlag, 0, 1> {};
I also like to put "// Type, start, size." or similar on the first declaration
in the group or before it.  It's usually obvious, but it saves having to
remember the order of the two ints and whether size or end position is encoded.

http://codereview.chromium.org/606063/diff/3002/2004#newcode524
src/codegen.h:524: class ArgcBits: public BitField<int, 2, 30> {};
32 - 2?  Might be useful to document that this is not a 30 bit field but "the
rest".

http://codereview.chromium.org/606063/diff/3002/2005
File src/runtime.cc (right):

http://codereview.chromium.org/606063/diff/3002/2005#newcode1508
src/runtime.cc:1508: static const int kStringBuilderConcatHelperPositionBits =
21;
32 - 11?

http://codereview.chromium.org/606063/diff/3002/2005#newcode1517
src/runtime.cc:1517: typedef BitField<int, 11, 21>
StringBuilderSubstringPosition;
32 - 11?

http://codereview.chromium.org/606063/diff/3002/2007
File src/utils.h (right):

http://codereview.chromium.org/606063/diff/3002/2007#newcode162
src/utils.h:162: return (((1U << (size + shift - 1)) << 1) - (1U << shift));
This code assumes size and shift are not both zero (reasonable).  If that's the
case then shift can't be 32 and you could write:

((1U << shift) << size) - (1U << shift)

?

http://codereview.chromium.org/606063/diff/3002/2007#newcode175
src/utils.h:175: return static_cast<T>((value >> shift) & (((1U << (size - 1))
<< 1) - 1));
Hairy.  Is it worse to write:

(value & mask()) >> shift

?

[fschneider, 2010-02-17 12:00:31]

http://codereview.chromium.org/606063/diff/3002/2004
File src/codegen.h (right):

http://codereview.chromium.org/606063/diff/3002/2004#newcode521
src/codegen.h:521: // Minor key encoding in 32 bits AAAAAAAAAAAAAAAAAAAAAFI
A(rgs)F(lag)I(nloop).
On 2010/02/17 10:16:33, Kevin Millikin wrote:
> I think these comments should go.  It's impossible to count the A's, and the
bit
> field declarations are self-documenting and authoritative (and nobody cares
> about the specific bit positions if they use the accessors anyway).
> 
> I think a simple
> 
> // Minor key is encoded in 32 bits.
> 
> is better.

Done.

http://codereview.chromium.org/606063/diff/3002/2004#newcode522
src/codegen.h:522: class InLoopBits: public BitField<InLoopFlag, 0, 1> {};
On 2010/02/17 10:16:33, Kevin Millikin wrote:
> I also like to put "// Type, start, size." or similar on the first declaration
> in the group or before it.  It's usually obvious, but it saves having to
> remember the order of the two ints and whether size or end position is
encoded.

Done.

http://codereview.chromium.org/606063/diff/3002/2004#newcode524
src/codegen.h:524: class ArgcBits: public BitField<int, 2, 30> {};
On 2010/02/17 10:16:33, Kevin Millikin wrote:
> 32 - 2?  Might be useful to document that this is not a 30 bit field but "the
> rest".

Done.

http://codereview.chromium.org/606063/diff/3002/2005
File src/runtime.cc (right):

http://codereview.chromium.org/606063/diff/3002/2005#newcode1508
src/runtime.cc:1508: static const int kStringBuilderConcatHelperPositionBits =
21;
On 2010/02/17 10:16:33, Kevin Millikin wrote:
> 32 - 11?

I'm changing this back since the value is actually converted to a smi later.

http://codereview.chromium.org/606063/diff/3002/2005#newcode1517
src/runtime.cc:1517: typedef BitField<int, 11, 21>
StringBuilderSubstringPosition;
On 2010/02/17 10:16:33, Kevin Millikin wrote:
> 32 - 11?
 Same here.

http://codereview.chromium.org/606063/diff/3002/2007
File src/utils.h (right):

http://codereview.chromium.org/606063/diff/3002/2007#newcode162
src/utils.h:162: return (((1U << (size + shift - 1)) << 1) - (1U << shift));
On 2010/02/17 10:16:33, Kevin Millikin wrote:
> This code assumes size and shift are not both zero (reasonable).  If that's
the
> case then shift can't be 32 and you could write:
> 
> ((1U << shift) << size) - (1U << shift)
> 
> ?

Done. There is one (not too useful) border case [shift=0, size=32] that would
still give a warning. But that's ok since we should not use a bitfield in that
case anyway.

http://codereview.chromium.org/606063/diff/3002/2007#newcode175
src/utils.h:175: return static_cast<T>((value >> shift) & (((1U << (size - 1))
<< 1) - 1));
On 2010/02/17 10:16:33, Kevin Millikin wrote:
> Hairy.  Is it worse to write:
> 
> (value & mask()) >> shift
> 
> ?

Done. Good idea to reuse mask().