LGTM from my end with a few nits. The one big comment about runtime function
moving from builtins.cc to runtime.cc we already discussed offline. Also
architecture ports are still missing.
https://codereview.chromium.org/14576005/diff/6001/src/builtins-decls.h
File src/builtins-decls.h (right):
https://codereview.chromium.org/14576005/diff/6001/src/builtins-decls.h#newcode36
src/builtins-decls.h:36: DECLARE_RUNTIME_FUNCTION(MaybeObject*,
ArrayConstructor_StubFailure);
As discussed offline: I would really like to see these two runtime
functions be moved into runtime.cc, but that is a job for a follow-up
CL. But we might want to leave a TODO here in that regard.
https://codereview.chromium.org/14576005/diff/6001/src/builtins.cc
File src/builtins.cc (right):
https://codereview.chromium.org/14576005/diff/6001/src/builtins.cc#newcode197
src/builtins.cc:197: static MaybeObject*
CommonArrayConstructorStubFailure(
nit: Can we call this "ArrayConstructorStubFailureCommon", that would be
more in line with the rest of the code base.
https://codereview.chromium.org/14576005/diff/6001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/14576005/diff/6001/src/code-stubs-hydrogen.cc#newcode563
src/code-stubs-hydrogen.cc:563: if (argument_class == NONE) {
Can we model that as a switch without a default case? This way the
compiler will puke when we extend the enum above.
https://codereview.chromium.org/14576005/diff/6001/src/code-stubs-hydrogen.cc#newcode580
src/code-stubs-hydrogen.cc:580: if (argument_class == NONE) {
Likewise.
https://codereview.chromium.org/14576005/diff/6001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/14576005/diff/6001/src/hydrogen.cc#newcode1895
src/hydrogen.cc:1895: builder_(builder),
nit: Indentation is off.
https://codereview.chromium.org/14576005/diff/6001/src/hydrogen.cc#newcode1910
src/hydrogen.cc:1910: builder_(builder),
nit: Indentation is off.
https://codereview.chromium.org/14576005/diff/6001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/14576005/diff/6001/src/hydrogen.h#newcode1271
src/hydrogen.h:1271: HValue* InternalEmitMapCode();
nit: Can we call this "EmitInternalMapCode()", that would be more in
line with the other naming schemes.
https://codereview.chromium.org/14576005/
--
--
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/groups/opt_out.
