Overall LGTM. Main style comments:
- clean-up how you overflow arguments to fit what we generally do in V8 (see
inline comment)
- Replace Handle<...> value = handle(...) with Handle<...> value(...);
https://codereview.chromium.org/228333003/diff/60001/src/elements.cc
File src/elements.cc (right):
https://codereview.chromium.org/228333003/diff/60001/src/elements.cc#newcode1461
src/elements.cc:1461: MUST_USE_RESULT static Handle<Object>
SetLengthWithoutNormalize(
Remove MUST_USE_RESULT
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3163
src/objects.cc:3163: Handle<AccessorInfo> entry =
handle(AccessorInfo::cast(callbacks->get(i)));
Handle<AccessorInfo> entry(AccessorInfo::cast(...));
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3164
src/objects.cc:3164: Handle<Name> key =
handle(Name::cast(entry->name()));
Same.
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3380
src/objects.cc:3380: Handle<Map> start_map = handle(object->map());
Handle<Map> start_map(object->map());
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode3410
src/objects.cc:3410: Handle<Map> closest_map =
handle(FindClosestElementsTransition(*map, kind));
closest_map(FindClosest...);
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode4771
src/objects.cc:4771: Handle<FixedArrayBase> elements =
handle(map->GetInitialElements());
elements(map->GetInitial...)
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode6861
src/objects.cc:6861: // Disallow gc until after
result->InitializeDescriptors() is called.
Splitting this up isn't necessary (doesn't help). We just need to
disallow-heap-allocation between new_descriptors->Append in the other
branch; and InitializeDescriptors below. Perhaps it makes sense to
rather copy result->SetBackPointer(*map) and
result->InitializeDescriptors(*new_descriptors) into both branches; and
have a DisallowHeapAllocation in the true case before Append.
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7057
src/objects.cc:7057: Handle<DescriptorArray> descriptors =
handle(map->instance_descriptors());
descriptors(map->instance..);
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7097
src/objects.cc:7097: Handle<DescriptorArray> descriptors =
handle(map->instance_descriptors());
descriptors(map->...)
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7138
src/objects.cc:7138: Handle<DescriptorArray> old_descriptors =
handle(map->instance_descriptors());
old_descriptors(map->...)
https://codereview.chromium.org/228333003/diff/60001/src/objects.cc#newcode7146
src/objects.cc:7146: return Map::CopyReplaceDescriptor(map,
old_descriptors, descriptor,
nit: In general, whenever arguments overflow, either put one per line,
or break after the (, as:
Map::CopyReplaceDescriptor(
map, old_descriptors, ...);
It just makes the code more readable if we follow the same pattern
everywhere...
https://codereview.chromium.org/228333003/
--
--
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/d/optout.
