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();
On 2013/06/05 14:53:24, Jakob wrote:
nit: {}
Done.
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();
On 2013/06/05 14:53:24, Jakob wrote:
nit: space before '?'. If that causes a line break, align ':' with
'?'.
Done.
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();
On 2013/06/05 14:53:24, Jakob wrote:
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.)
Added a comment to types.h. Left comparisons as is, if that's okay,
because I find them far more readable that way (esp because they are
more symmetric).
https://codereview.chromium.org/16154027/diff/16001/src/types.cc#newcode261
src/types.cc:261: return *union_get(unioned, 0);
On 2013/06/05 14:53:24, Jakob wrote:
nit: {}
Done.
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
On 2013/06/05 14:53:24, Jakob wrote:
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.
Yes, let's add stuff once we get there.
https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode133
src/types.h:133: kNull = 1<<0,
On 2013/06/05 14:53:24, Jakob wrote:
nit: any reason not to have spaces around the operator '<<'?
Done.
https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode171
src/types.h:171: Handle<Type> handle_via(Type* type) {
On 2013/06/05 14:53:24, Jakob wrote:
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.
Done.
https://codereview.chromium.org/16154027/diff/16001/src/types.h#newcode189
src/types.h:189: int LubBitset();
On 2013/06/05 14:53:24, Jakob wrote:
short comment "Least upper bound"? You can even add that on the same
line.
Done.
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));
On 2013/06/05 14:53:24, Jakob wrote:
nit: {}
Done.
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));
On 2013/06/05 14:53:24, Jakob wrote:
nit: {}
Done.
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));
On 2013/06/05 14:53:24, Jakob wrote:
nit: {}
Done.
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));
On 2013/06/05 14:53:24, Jakob wrote:
nit: {}
Done.
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));
On 2013/06/05 14:53:24, Jakob wrote:
nit: {} (and again below)
Done.
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.
