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