lgtm once nits are addressed.
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__ } \
On 2015/07/27 17:09:53, oth wrote:
On 2015/07/27 14:56:57, rmcilroy (OOO until 10th Aug) wrote:
> 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?
Yes, C++ will initialize these to zero provided at least one value is
given. For
readability the declarations could include kNone as the last operand
type or use
templates to pad out the arguments to explicitly include 0's.
OK sounds good.
The current nit that grates here is counting the operands at runtime
rather than
compile time. The current pattern is more readable, but would be
better to
determine at compile time.
I'm not too worried about this since it will only be run when building
the interpreter bytecode handlers (usually at snapshot generation time),
but if you are worried you could build a separate table of operand
counts using a hacky trick like:
#define NUM_VAR_ARGS(...) (sizeof((uint8_t[]){__VA_ARGS__})
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#newcode57
src/interpreter/interpreter.cc:57: void
Interpreter::DoLoadSmi8(compiler::InterpreterAssembler* assembler) {
On 2015/07/27 16:59:00, oth wrote:
On 2015/07/27 14:56:58, rmcilroy (OOO until 10th Aug) wrote:
> Is LoadLiteralSmi8 too wordy? (honest question).
There are two issues here - how handlers are named and what the
disassembly
looks like. Personally, I'd lean towards compact and easy to scan
disassembler
output. Handlers could have the more descriptive names and the
disassembly a
compact and easy scan format by adding an extra argument to
DECLARE_BYTECODE?
No, I'd rather have the handlers named the same as the bytecode in
disassembly. This is fine I think.
https://codereview.chromium.org/1257543003/diff/60001/src/compiler/interpreter-assembler.h
File src/compiler/interpreter-assembler.h (right):
https://codereview.chromium.org/1257543003/diff/60001/src/compiler/interpreter-assembler.h#newcode44
src/compiler/interpreter-assembler.h:44: // Returns the bytecode
argument |index| for the current bytecode.
update comment too please.
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecodes.cc#newcode20
src/interpreter/bytecodes.cc:20: ,
nit - comma on the line above?
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecodes.h#newcode18
src/interpreter/bytecodes.h:18: // The list of operands used by
bytecodes.
/s/operands/operand types/
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecodes.h#newcode29
src/interpreter/bytecodes.h:29:
nit - extra newline
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecodes.h#newcode41
src/interpreter/bytecodes.h:41:
nit - extra newline
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/bytecodes.h#newcode63
src/interpreter/bytecodes.h:63: // Returns bytecode for |value|,
checking validity in the process.
nit - remove the ", checking validity in the process" since this is now
a DCHECK.
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/interpreter.cc#newcode50
src/interpreter/interpreter.cc:50: // argument.
/s/the register index specified by the bytecode's argument/the
destination register
https://codereview.chromium.org/1257543003/diff/60001/src/interpreter/interpreter.cc#newcode62
src/interpreter/interpreter.cc:62: UNIMPLEMENTED();
nit - just make this //TODO(rmcilroy) Implement LoadSmi8 for now.
https://codereview.chromium.org/1257543003/diff/60001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1257543003/diff/60001/src/objects.cc#newcode11637
src/objects.cc:11637: os << bytecode << "\n";
could we add the formatted versions of the arguments after the bytecode
- e.g., r1 for register 1 and #123 for literal SMI 123, etc? (fine to do
in another CL).
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.
