CL looks plausible to me. However, my understanding of the code dependency
(and
GC) stuff is rather vague, so you might want to get a second reviewer for
those
parts.
https://codereview.chromium.org/16925008/diff/37001/src/types.cc
File src/types.cc (right):
https://codereview.chromium.org/16925008/diff/37001/src/types.cc#newcode203
src/types.cc:203: case JS_SET_TYPE:
JS_SET/MAP should actually go up to the other ordinary JS object types.
https://codereview.chromium.org/16925008/diff/37001/src/types.h
File src/types.h (right):
https://codereview.chromium.org/16925008/diff/37001/src/types.h#newcode214
src/types.h:214: kAny = kOddball | kNumber | kAllocated | kInternal,
I can't yet make up my mind whether Internal should really be included
in Any (or Allocated?), or if we should have a different name for that
union. But let's leave it like that for now.
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs-hydrogen.cc#newcode780
src/code-stubs-hydrogen.cc:780: Handle<Object> hole =
Handle<Object>(isolate()->heap()->the_hole_value(),
Nit: can use constructor init syntax to avoid the duplicate type
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h#newcode527
src/code-stubs.h:527: explicit StoreGlobalStub(StrictModeFlag
strict_mode,
The 'explicit' is unnecessary here.
Also, parameters fit on one line.
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h#newcode542
src/code-stubs.h:542: return bit_field_;
Nit: I believe this fits on the prev line
https://codereview.chromium.org/16925008/diff/39001/src/heap.h
File src/heap.h (right):
https://codereview.chromium.org/16925008/diff/39001/src/heap.h#newcode264
src/heap.h:264: V(cell_value_string, "%call_value")
\
Typo? %cell_value
https://codereview.chromium.org/16925008/diff/39001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/hydrogen.cc#newcode6561
src/hydrogen.cc:6561: AddSoftDeoptimize(MUST_EMIT_SOFT_DEOPT);
I'm a bit at a loss why this flag is needed.
https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-resolver-ia32.cc
File src/ia32/lithium-gap-resolver-ia32.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-resolver-ia32.cc#newcode320
src/ia32/lithium-gap-resolver-ia32.cc:320: int32_t upper =
static_cast<int32_t>(int_val >> (kBitsPerInt));
Nit: redundant parens
https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-resolver-ia32.cc#newcode336
src/ia32/lithium-gap-resolver-ia32.cc:336: cgen_->PopX87();
Hm, why this?
https://codereview.chromium.org/16925008/diff/39001/src/ic.cc
File src/ic.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/ic.cc#newcode2098
src/ic.cc:2098: Handle<Object> value) {
Where is this parameter used?
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode10132
src/objects.cc:10132: if (result == NULL) return NULL;
Nit: could avoid the extra return using ?:
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode10218
src/objects.cc:10218: Cell* replace_with) {
Nit: this fits on the previous line
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15823
src/objects.cc:15823: Type* PropertyCell::UnionType(Object* value) {
I'm not sure I fully understand the logic of this function. Intuitively,
it seems more complicated than it should be (I had to draw a state
transition diagram to follow it :) ).
Also, 'UnionType' is a bit of an odd name, since it never computes a
proper union. And it doesn't make it particularly obvious that it has
the side effect of causing deopts. How about something like
'UpdateType'?
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15829
src/objects.cc:15829: if (new_type->Is(old_type) && !value->IsTheHole())
{
There is repeated special-casing for the hole. Maybe that can be avoided
by initialising new_type as:
Handle<Type> new_type(value->IsTheHole() ? Type::Any() :
Type::Constant(value_handle, isolate), isolate);
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15836
src/objects.cc:15836: if (*old_type == Type::None()) {
Please don't rely on pointer equality for types. This should be
"old_type->Is(Type::None())".
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15837
src/objects.cc:15837: if (new_type->IsConstant() && !value->IsTheHole())
{
Given that new_type is initialised to a Constant above, the first part
of the condition seems redundant. Moreover, if you initialise new_type
as I suggested above, I believe you can return new_type unconditionally.
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15840
src/objects.cc:15840: } else if (old_type->IsConstant() &&
old_type->AsConstant()->IsUndefined()) {
I think you can just as well test "old_type->Is(Type::Undefined())" for
this.
Also, IIUC, you allow overriding undefined with the hole here.
https://codereview.chromium.org/16925008/diff/39001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/16925008/diff/39001/src/objects.h#newcode4693
src/objects.h:4693: void ReplaceNthObject(int n, Map* match_map, Object*
replace_with);
As we discussed earlier, this seems rather fragile. Is the plan still
to have an explicit notion of parameters eventually?
https://codereview.chromium.org/16925008/diff/39001/src/objects.h#newcode8597
src/objects.h:8597: void set_value_infer_type(Object* value,
This function can also trigger deopts, so I'd prefer not giving it a
seemingly innocent lower-case name.
https://codereview.chromium.org/16925008/diff/39001/src/types.cc
File src/types.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/types.cc#newcode133
src/types.cc:133: if (value->IsTrue() || value->IsFalse()) return
kBoolean;
You might want to add a case sending the hole to type kAny here. So far,
I've basically ignored the hole, because it's an internal type.
https://codereview.chromium.org/16925008/
--
--
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.
