https://codereview.chromium.org/18881004/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):
https://codereview.chromium.org/18881004/diff/1/src/arm/code-stubs-arm.cc#newcode269
src/arm/code-stubs-arm.cc:269: descriptor->deoptimization_handler_ =
Runtime::FunctionForId(
Yikes. In general, you should access members of the Stub during
descriptor initialization, this seems fragile. I would much prefer
having one descriptor per stub class and one stub class per descriptor .
https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#newcode937
src/code-stubs-hydrogen.cc:937: set_current_block(NULL);
I think it's cleaner if you return value here and then the code below
doesn't have to be in an else.
https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#newcode945
src/code-stubs-hydrogen.cc:945: AddStore(array, HObjectAccess::ForMap(),
target_map);
Why the special casing here? Either the stub should have no side-effects
and defer to the runtime, or it should handle the case completely. Why
can you not just use BuildUncheckedMonomorphicElementAccess with the
"holeyified" elements kind? Also, this case applies to all packed->holey
transitions, I think, e.g. the if should be:
if (GetPackedElementsKind(from_kind) == GetPackedElementsKind(to_kind)
&&
IsFastPackedElementsKind(from_kind) &&
IsFastHoleyElementsKind(to_kind)) {
...
https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#newcode956
src/code-stubs-hydrogen.cc:956: IsFastObjectElementsKind(to_kind))) {
Can you create a predicate in elements-kind.h to the effect
DoesTransitionChangeElementsBufferFormat or something similar?
https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#newcode963
src/code-stubs-hydrogen.cc:963:
if_fixed.If<HCompareNumericAndBranch>(elements_length,
You can use IfNot here and then put the meat of the code below in the
"Then" and omit the "Else"
https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#newcode974
src/code-stubs-hydrogen.cc:974: BuildNewSpaceArrayCheck(elements_length,
to_kind);
Why can't you use the helper function "BuildGrowElementsCapacity" for
this line and the following ~10 or so?
https://codereview.chromium.org/18881004/diff/1/src/code-stubs-hydrogen.cc#newcode996
src/code-stubs-hydrogen.cc:996:
BuildUncheckedMonomorphicElementAccess(array, key, value, NULL,
You can probably factor this out to only be called once for both the
elements format change and simple map changes. For simple map changes,
you will have to first do a
to_kind = GetHoleyElementsKind(to_kind);
https://codereview.chromium.org/18881004/diff/1/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/18881004/diff/1/src/code-stubs.h#newcode2214
src/code-stubs.h:2214: class ElementsTransitionAndStoreStub : public
HydrogenCodeStub {
I kind of like the existing invariant that each stub has it's own
descriptor and there is always a 1-to-1 mapping. It think it would be
cleaner if you make this a superclass with two subclasses, one for
strict and one for non-strict. then you could override the script_mode()
method (make it virtual), and everything else should just work.
https://codereview.chromium.org/18881004/diff/1/src/code-stubs.h#newcode2259
src/code-stubs.h:2259: StoreModeBits::encode(store_mode_);
If you choose to do it this way, use the accessor functions.
https://codereview.chromium.org/18881004/
--
--
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.
