First round of comments, cool stuff!
https://codereview.chromium.org/15094018/diff/17001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/15094018/diff/17001/src/code-stubs-hydrogen.cc#newcode320
src/code-stubs-hydrogen.cc:320: AddInstruction(new(zone)
HLoadKeyed(literals,
literals is just used there, you could just use the original code.
https://codereview.chromium.org/15094018/diff/17001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/15094018/diff/17001/src/flag-definitions.h#newcode270
src/flag-definitions.h:270: DEFINE_bool(optimize_constructed_arrays,
false,
Should be true.
https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode1657
src/hydrogen.cc:1657: }
ASSERT if boilerplate and allocation_site->boilerplate are the same
https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode5885
src/hydrogen.cc:5885: Handle<FixedArray> literals(closure->literals(),
isolate());
Literals can be removed.
https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode6018
src/hydrogen.cc:6018: // AllocationSite from the full code gen
why can't you reuse the allocation site generated in full code gen?
https://codereview.chromium.org/15094018/diff/17001/src/objects-printer.cc
File src/objects-printer.cc (right):
https://codereview.chromium.org/15094018/diff/17001/src/objects-printer.cc#newcode1122
src/objects-printer.cc:1122: PrintF(out, " - payload: ");
Why don't we call payload AllocationSite? That would carry more
information.
https://codereview.chromium.org/15094018/diff/17001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7462
src/objects.h:7462: DECL_ACCESSORS(payload, Object)
Can we rename payload to something more meaningful? Like
transition_data...?
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7467
src/objects.h:7467:
set_payload(Smi::FromInt(GetInitialFastElementsKind()));
You can call SetElementsKindPayload there.
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7468
src/objects.h:7468: }
declaration order: Typedefs and Enums, Constants (static const data
members), Constructors, Destructor, Methods including static methods,
Data Members (except static const data members)
Moreover can we keep the static methods together?
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7491
src/objects.h:7491: return !payload()->IsSmi(); // payload is a
boilerplate
remove comment or make it clear.
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7499
src/objects.h:7499: class AllocationSiteInfo: public Struct {
declaration order... I just saw that it is not consistent in our code
base but the google coding style guide suggest to do that. Let's be a
good example.
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7511
src/objects.h:7511: static const int kPayloadOffset =
HeapObject::kHeaderSize;
Do you think it makes sense to rename kPayloadOffset to
kAllocationSiteOffset? I do not think that we are going to add more
fields to AllocationSiteInfo.
https://codereview.chromium.org/15094018/diff/17001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/15094018/diff/17001/src/runtime.cc#newcode532
src/runtime.cc:532: RUNTIME_FUNCTION(MaybeObject*,
Runtime_CreateArrayLiteralShallow) {
Can we refactor Runtime_CreateArrayLiteral and
Runtime_CreateArrayLiteralShallow. They share quite some similarities.
https://codereview.chromium.org/15094018/
--
--
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.
