This looks much better to me. A couple of comments, most of which are nits.
https://codereview.chromium.org/1257543003/diff/40001/src/compiler/interpreter-assembler.cc
File src/compiler/interpreter-assembler.cc (right):
https://codereview.chromium.org/1257543003/diff/40001/src/compiler/interpreter-assembler.cc#newcode92
src/compiler/interpreter-assembler.cc:92: Node*
InterpreterAssembler::BytecodeArg(int delta) {
Could you change this to BytecodeOperand as well please.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.cc#newcode19
src/interpreter/bytecodes.cc:19: { __VA_ARGS__ } \
Does C++ always zero-initialize entries in an array? i.e., does:
{ Operand::kImm8 }
turn into
{ 1, 0, 0 }
here implicitly, or are entries [1] and [2] undefined?
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.cc#newcode49
src/interpreter/bytecodes.cc:49: CHECK(bytecode <= Bytecode::kLast);
DCHECK
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.cc#newcode56
src/interpreter/bytecodes.cc:56: CHECK(bytecode <= Bytecode::kLast);
DCHECK
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.cc#newcode70
src/interpreter/bytecodes.cc:70: CHECK(bytecode <= Bytecode::kLast && i
< NumberOfOperands(bytecode));
DCHECK
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode19
src/interpreter/bytecodes.h:19: #define OPERAND_LIST(V) \
nit - OPERAND_TYPE_LIST
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode24
src/interpreter/bytecodes.h:24: enum class Operand : uint8_t {
nit - OperandType
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode39
src/interpreter/bytecodes.h:39: V(Return, Operand::kNone)
nit - can we move this up just below OPERAND_LIST just to make these
declarations obvious.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode62
src/interpreter/bytecodes.h:62: // Returns bytecode for |value|,,
checking validity in the process.
remove extra ','
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode68
src/interpreter/bytecodes.h:68: // Return the i-th operand of
|bytecode|.
Return the type of the i-th operand of |bytecode|
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/bytecodes.h#newcode69
src/interpreter/bytecodes.h:69: static const Operand GetOperand(Bytecode
bytecode, int i);
GetOperandType
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpreter.cc#newcode49
src/interpreter/interpreter.cc:49: void
Interpreter::DoLoadSmi0(compiler::InterpreterAssembler* assembler) {
I think we should keep this as LoadLiteral0 so that it is distinct from
LoadSmi8 (which doesn't actually load '8' but loads 8bits.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpreter.cc#newcode56
src/interpreter/interpreter.cc:56: // LoadSmi8 dst, imm8
nit - could you update all the comments to have this form and add a
small commentary below it like the other comments.
https://codereview.chromium.org/1257543003/diff/40001/src/interpreter/interpreter.cc#newcode57
src/interpreter/interpreter.cc:57: void
Interpreter::DoLoadSmi8(compiler::InterpreterAssembler* assembler) {
Is LoadLiteralSmi8 too wordy? (honest question).
https://codereview.chromium.org/1257543003/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
