Thanks both. Feedback incorporated.
Waiting on https://codereview.chromium.org/1289863003/ before committing.
https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecode-generator.cc
File src/interpreter/bytecode-generator.cc (right):
https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecode-generator.cc#newcode8
src/interpreter/bytecode-generator.cc:8: #include
"src/interpreter/bytecode-generator.h"
On 2015/08/18 08:17:16, Michael Starzinger wrote:
nit: Please move the include of bytecode-generator.h all the way to
the top of
the include list (above "stack" even).
Done.
https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecode-generator.cc#newcode28
src/interpreter/bytecode-generator.cc:28:
DCHECK(scope()->is_function_scope());
On 2015/08/18 08:17:16, Michael Starzinger wrote:
I assume this is just a temporary guard, because it won't hold for all
compilation units.
Yes, added a comment for other readers.
https://codereview.chromium.org/1294543002/diff/20001/src/interpreter/bytecode-generator.cc#newcode101
src/interpreter/bytecode-generator.cc:101:
stmt->expression()->Accept(this);
On 2015/08/18 08:17:16, Michael Starzinger wrote:
nit: Better use Visit(stmt) here. Performs stack-overflow/bailout
check and
allows to override BytecodeGenerator::Visit later if needed.
Done.
https://codereview.chromium.org/1294543002/diff/20001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):
https://codereview.chromium.org/1294543002/diff/20001/tools/gyp/v8.gyp#newcode802
tools/gyp/v8.gyp:802: '../../src/interpreter/bytecode-generator.cc',
On 2015/08/18 08:17:16, Michael Starzinger wrote:
nit: This will also require BUILD.gn to be adapted.
Done.
https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc
File test/cctest/interpreter/test-bytecode-generator.cc (right):
https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode23
test/cctest/interpreter/test-bytecode-generator.cc:23:
i::FLAG_print_bytecode = false;
On 2015/08/18 10:31:31, rmcilroy wrote:
nit - remove the "i::FLAG_print_bytecode = false"? It should be false
by default
and if you don't force it to false then you can pass
"--print_bytecode" to the
tests if you want to see the bytecode for a particular test.
Done. It was an explicit clue on debugging problems here.
https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode53
test/cctest/interpreter/test-bytecode-generator.cc:53:
On 2015/08/18 10:31:31, rmcilroy wrote:
nit - extra newline
Done.
https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode112
test/cctest/interpreter/test-bytecode-generator.cc:112: }}};
On 2015/08/18 10:31:31, rmcilroy wrote:
indentation seems a little unusual here - is "git cl format"
responsable?
Yes.
https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode123
test/cctest/interpreter/test-bytecode-generator.cc:123: }
On 2015/08/18 10:31:31, rmcilroy wrote:
nit - extra newline below
Done.
https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode124
test/cctest/interpreter/test-bytecode-generator.cc:124: }
On 2015/08/18 10:31:31, rmcilroy wrote:
nit - comments on closing namespace brackets.
Done.
https://codereview.chromium.org/1294543002/
--
--
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.
