Cool stuff! Mostly nits:
https://codereview.chromium.org/1316213008/diff/80001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1316213008/diff/80001/src/objects.cc#newcode6492
src/objects.cc:6492: int KeyAccumulator::GetLength() { return length_; }
I think it's better to implement simple get/set methods inline.
https://codereview.chromium.org/1316213008/diff/80001/src/objects.cc#newcode6562
src/objects.cc:6562: set_ = ObjectHashTable::New(isolate_, length_);
On 2015/09/14 at 07:34:04, cbruni wrote:
Technically this should be a set but we only have a reasonable dict
implementation. So we waste a bit of memory here in the case we fall
back to dict.
Maybe it's worth implementing an ObjectHashSet. It should be pretty
straightforward.
https://codereview.chromium.org/1316213008/diff/80001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/1316213008/diff/80001/src/objects.h#newcode10474
src/objects.h:10474: : isolate_(isolate), keys_(), set_(), length_(0) {}
keys_() and set_() are not really necessary here.
https://codereview.chromium.org/1316213008/diff/120001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1316213008/diff/120001/src/objects.cc#newcode6500
src/objects.cc:6500: int KeyAccumulator::GetLength() { return length_; }
Please move this implementation to the class definition.
https://codereview.chromium.org/1316213008/diff/120001/src/objects.cc#newcode15545
src/objects.cc:15545: // Read the enxisting bucket values.
s/enxisting/existing/
https://codereview.chromium.org/1316213008/diff/140001/src/elements.cc
File src/elements.cc (right):
https://codereview.chromium.org/1316213008/diff/140001/src/elements.cc#newcode876
src/elements.cc:876: virtual void
AddElementsToFixedArrayWithAccumulator(
WDYT about AddElementsToKeyAccumulator() name?
https://codereview.chromium.org/1316213008/diff/140001/src/elements.cc#newcode887
src/elements.cc:887: Handle<Object> value =
ElementsAccessorSubclass::GetImpl(from, i);
HasEntryImpl() returned true, so value must not be hole here.
https://codereview.chromium.org/1316213008/diff/140001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1316213008/diff/140001/src/objects.cc#newcode6736
src/objects.cc:6736: FixedArray::NON_SYMBOL_KEYS);
Why do you use a different filter than in the old code?
https://codereview.chromium.org/1316213008/diff/140001/src/objects.cc#newcode15432
src/objects.cc:15432: Isolate* isolate = table->GetIsolate();
Spurious change.
https://codereview.chromium.org/1316213008/diff/140001/src/objects.cc#newcode15447
src/objects.cc:15447: Isolate* isolate = table->GetIsolate();
Spurious change.
https://codereview.chromium.org/1316213008/diff/140001/src/objects.cc#newcode15659
src/objects.cc:15659: table->set(new_index + 1,
Smi::FromInt(previous_entry));
I think "+ kChainOffset" is better than "+ 1" for readability.
https://codereview.chromium.org/1316213008/diff/140001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/1316213008/diff/140001/src/objects.h#newcode3691
src/objects.h:3691: Object* next_entry = get(EntryToIndex(entry) +
entrysize);
s/entrysize/kChainOffset/g (for readability).
https://codereview.chromium.org/1316213008/diff/140001/src/objects.h#newcode3693
src/objects.h:3693: }
Please add a blank line here.
https://codereview.chromium.org/1316213008/diff/140001/src/runtime/runtime-object.cc
File src/runtime/runtime-object.cc (right):
https://codereview.chromium.org/1316213008/diff/140001/src/runtime/runtime-object.cc#newcode1004
src/runtime/runtime-object.cc:1004: offset += 100;
int end = min(offset, length);
https://codereview.chromium.org/1316213008/diff/140001/src/runtime/runtime-object.cc#newcode1005
src/runtime/runtime-object.cc:1005: for (int i = offset - 100; i <
offset && i < length; i++) {
...; i < end; ...
https://codereview.chromium.org/1316213008/
--
--
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.
