LGTM if all comments are addressed from my end. Please also wait on
Benedikt's
sign-off. I like this a lot. Thanks!
https://codereview.chromium.org/1306993003/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):
https://codereview.chromium.org/1306993003/diff/20001/src/bootstrapper.cc#newcode1847
src/bootstrapper.cc:1847:
native_context()->set_reflect_construct(*construct);
Woot! Thanks!
https://codereview.chromium.org/1306993003/diff/20001/src/compiler/ast-graph-builder.cc
File src/compiler/ast-graph-builder.cc (right):
https://codereview.chromium.org/1306993003/diff/20001/src/compiler/ast-graph-builder.cc#newcode2552
src/compiler/ast-graph-builder.cc:2552:
environment()->Push(jsgraph()->UndefinedConstant());
nit: Let's add a trailing "// receiver" comment, or alternatively use a
local variable called "receiver_value", both are fine with me.
https://codereview.chromium.org/1306993003/diff/20001/src/contexts.h
File src/contexts.h (left):
https://codereview.chromium.org/1306993003/diff/20001/src/contexts.h#oldcode132
src/contexts.h:132: #define NATIVE_CONTEXT_IMPORTED_FIELDS_FOR_PROXY(V)
\
Yay! I like that this is gone and has been merged into the above list.
https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen-instructions.cc#newcode1102
src/hydrogen-instructions.cc:1102: os << function()->name << " ";
Is "this" guaranteed to point to a runtime call or do we need
"is_jsruntime()" here?
https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/1306993003/diff/20001/src/hydrogen.cc#newcode11102
src/hydrogen.cc:11102: if (call->function()->function_id !=
Runtime::kInlineClassOf) return false;
Is "call" guaranteed to point to a runtime function here or do we need a
"!call->is_jsruntime()" here to guard that? Because ClusterFuzz will
find that one for sure. :)
https://codereview.chromium.org/1306993003/diff/20001/src/prettyprinter.cc
File src/prettyprinter.cc (right):
https://codereview.chromium.org/1306993003/diff/20001/src/prettyprinter.cc#newcode798
src/prettyprinter.cc:798: Print("%%%s\n", node->function()->name);
Is node guaranteed to have a runtime function or is "is_jsruntime()"
needed?
https://codereview.chromium.org/1306993003/diff/20001/src/prettyprinter.cc#newcode1526
src/prettyprinter.cc:1526: Print("NAME %s\n", node->function()->name);
Likewise.
https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):
https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.cc#newcode84
src/runtime/runtime.cc:84: if (kIntrinsicFunctions[i].intrinsic_type ==
RUNTIME &&
Is this needed? This looks fishy to me!
https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.h
File src/runtime/runtime.h (right):
https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.h#newcode9
src/runtime/runtime.h:9: #include "src/contexts.h"
nit: Include shouldn't be needed anymore, let's drop it.
https://codereview.chromium.org/1306993003/diff/20001/src/runtime/runtime.h#newcode1003
src/runtime/runtime.h:1003: // For CONTEXT functions this is the native
context index.
nit: Last line of the comment is obsolete now, let's drop it.
https://codereview.chromium.org/1306993003/
--
--
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.
