LGTM. Mostly nits. I guess we can't do much about the handle dereferencing
issue.
https://codereview.chromium.org/16154027/diff/16001/src/types.cc
File src/types.cc (right):
https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode41
src/types.cc:41: bitset |= union_get(unioned, i)->LubBitset();
nit: {}
https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode44
src/types.cc:44: Map* map = this->is_class()? *this->as_class() :
this->as_constant()->map();
nit: space before '?'. If that causes a line break, align ':' with '?'.
https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode124
src/types.cc:124: return this->is_class() && *this->as_class() ==
*that->as_class();
I think it would be cleaner to use Handle::is_identical_to() instead of
manually dereferencing and comparing.
Either way, as discussed offline dereferencing handles (implicitly or
explicitly) prevents us from using this during backgrounded compilation.
Maybe add a comment to that effect to make it obvious? (We already have
automated checks, but they're pretty hidden.)
https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode261
src/types.cc:261: return *union_get(unioned, 0);
nit: {}
https://codereview.chromium.org/16154027/diff/16001/src/types.h
File src/types.h (right):
https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode51
src/types.h:51: // Number = Smi \/ Double
As discussed offline, I think we'll need an Integer32 type before we can
use this to replace existing type description systems. But we can always
add that later.
https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode133
src/types.h:133: kNull = 1<<0,
nit: any reason not to have spaces around the operator '<<'?
https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode171
src/types.h:171: Handle<Type> handle_via(Type* type) {
I find it surprising that |type| is only used to retrieve its isolate.
I'd prefer making this more explicit: either change the signature to
take an Isolate* instead of a Type*, or rename to
handle_with_isolate_from(), or something like that.
https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode189
src/types.h:189: int LubBitset();
short comment "Least upper bound"? You can even add that on the same
line.
https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc
File test/cctest/test-types.cc (right):
https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc#newcode183
test/cctest/test-types.cc:183: CHECK_NE(AsBitset(*type1),
AsBitset(*type2));
nit: {}
https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc#newcode190
test/cctest/test-types.cc:190: CHECK_NE(AsBitset(*type1),
AsBitset(*type2));
nit: {}
https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc#newcode270
test/cctest/test-types.cc:270: CHECK_NE(0, AsBitset(*type1) &
AsBitset(*type2));
nit: {}
https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc#newcode279
test/cctest/test-types.cc:279: CHECK_EQ(0, AsBitset(*type1) &
AsBitset(*type2));
nit: {}
https://codereview.chromium.org/16154027/diff/16001/test/cctest/test-types.cc#newcode354
test/cctest/test-types.cc:354: CHECK_EQ(AsBitset(*type1),
AsBitset(*type2));
nit: {} (and again below)
https://codereview.chromium.org/16154027/
--
--
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.
