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(). http://codereview.chromium.org/606063 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
