Looks good. My main suggestion is whether we can move Register class to
bytecodes.h to keep all the logic together a bit more.
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecode-array-builder.h
File src/interpreter/bytecode-array-builder.h (right):
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecode-array-builder.h#newcode93
src/interpreter/bytecode-array-builder.h:93: class Register {
Would it be simpler just to move the Register class over to bytecodes.h?
(Genuine question)
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode123
src/interpreter/bytecodes.cc:123: uint8_t
Bytecodes::ParameterIndexToOperand(int index, int parameter_count) {
Could we make this a Register::FromParameterIndex() instead (and then
can use Register::ToOperand if we need the operand)? This is assuming we
can move Register class over to bytecodes.h as in my earlier comment.
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode126
src/interpreter/bytecodes.cc:126: DCHECK_GE(index, 0);
nit - DCHECK_LT(index, parameter_count)
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode144
src/interpreter/bytecodes.cc:144: return 128 + kLastParamRegisterIndex;
This is a bit confusing. Could you use kMinRegisterIndex with the -1
adjustment (from count to index) and mention that
kLastParamRegisterIndex is negative.
https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode186
src/interpreter/bytecodes.cc:186: os << "this";
nit - could we make it "<this>" or "|this|" or something to make it
clear this is a parameter? Maybe do the same with "<a0>" or "|a0|"as
well?
https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpreter/bytecodes-unittest.cc
File test/unittests/interpreter/bytecodes-unittest.cc (right):
https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpreter/bytecodes-unittest.cc#newcode24
test/unittests/interpreter/bytecodes-unittest.cc:24: int
parameter_counts[] = {7, 13, 99};
nit - could you just use a std::vector here instead of having to define
COUNT_OF?
https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpreter/bytecodes-unittest.cc#newcode43
test/unittests/interpreter/bytecodes-unittest.cc:43: for (int i = 0; i <
128; i++) {
nit - /s/128/kMaxRegisterIndex
https://codereview.chromium.org/1325983002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
