LGTM. Just nits and a suggestions.
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc
File src/compiler/bytecode-graph-builder.cc (right):
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode25
src/compiler/bytecode-graph-builder.cc:25: locals_count_(locals_count),
nit: Wouldn't it better fit the semantics if this were called
"register_count" instead?
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode134
src/compiler/bytecode-graph-builder.cc:134: // The additional count
items are for the context and closure. as
nit: Drop trailing "as".
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode139
src/compiler/bytecode-graph-builder.cc:139: int locals_count =
bytecode_array()->local_count();
nit: Likewise "register_count" here.
https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode172
src/compiler/bytecode-graph-builder.cc:172: for (int i = 0; i <
bytecode_array()->length(); i += last_bytecode_length) {
Would it be more readable to write the loop with less local variable? I
was thinking along the lines of the following. Just a suggestion, pick
whichever you think is more readable ...
int offset = 0
while (offset < bytecode_array()->length()) {
interpreter::Bytecode bytecode = bytecode_at(offset);
switch (...) { ... VisitFoo(offset + 1) ... }
offset += 1 + interpreter::Bytecodes::Size(bytecode);
}
https://codereview.chromium.org/1291693004/diff/140001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/1291693004/diff/140001/src/objects.h#newcode4163
src/objects.h:4163: inline int local_count() const;
nit: Wouldn't it better fit the semantics to call this "register_count"
instead?
https://codereview.chromium.org/1291693004/diff/140001/test/cctest/cctest.gyp
File test/cctest/cctest.gyp (right):
https://codereview.chromium.org/1291693004/diff/140001/test/cctest/cctest.gyp#newcode55
test/cctest/cctest.gyp:55:
'compiler/test-run-bytecode-graph-builder.cc',
nit: Please keep list alpha-sorted.
https://codereview.chromium.org/1291693004/diff/140001/test/cctest/compiler/test-run-bytecode-graph-builder.cc
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):
https://codereview.chromium.org/1291693004/diff/140001/test/cctest/compiler/test-run-bytecode-graph-builder.cc#newcode37
test/cctest/compiler/test-run-bytecode-graph-builder.cc:37:
isolate->factory()->undefined_value(), sizeof...(args),
Wow, "sizeof...", C++ never seizes to surprise me.
https://codereview.chromium.org/1291693004/diff/140001/test/cctest/compiler/test-run-bytecode-graph-builder.cc#newcode64
test/cctest/compiler/test-run-bytecode-graph-builder.cc:64:
i::FLAG_ignition_filter = kFunctionName;
This assignment could potentially trigger double-free situations.
Whenever we start passing --ingnition-filter=foo on the command line to
this cctest, the flags parsing will try to free the static const. I am
fine with leaving it as it is, just be warned that it will hit us
eventually.
https://codereview.chromium.org/1291693004/
--
--
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.
