lgtm with nits
https://codereview.chromium.org/167303005/diff/210001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/167303005/diff/210001/src/objects.cc#newcode2022
src/objects.cc:2022: // Allocate new instance descriptors with (name,
index) added
This comment is a bit weird. I'd just drop it entirely; unless you feel
strongly about indicating that CopyAddFieldDescriptor copies something
while adding a field descriptor :)
https://codereview.chromium.org/167303005/diff/210001/src/objects.cc#newcode2849
src/objects.cc:2849: new_field_type =
HeapType::Any(updated->GetIsolate());
Could you introduce a method similar to
new_representation.generalize(old_representation) for merging types?
Would be nice to not have this logic spread all over... (or at least,
have an abstraction for it).
https://codereview.chromium.org/167303005/diff/210001/src/objects.cc#newcode8213
src/objects.cc:8213: if (right_type->NowIs(left_type)) {
Same generalize logic would apply here
https://codereview.chromium.org/167303005/diff/210001/src/property.h
File src/property.h (right):
https://codereview.chromium.org/167303005/diff/210001/src/property.h#newcode202
src/property.h:202: return value->FitsRepresentation(representation());
return true;
Functions can take any value as argument.
https://codereview.chromium.org/167303005/
--
--
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.
