LGTM with comments.
https://codereview.chromium.org/176843006/diff/1/src/types.cc
File src/types.cc (right):
https://codereview.chromium.org/176843006/diff/1/src/types.cc#newcode158
src/types.cc:158: return kTaggedPtr & (
High-level comment: This assumes HeapNumber objects are immutable. This
doesn't hold for double-fields. Better think that through with Toon.
https://codereview.chromium.org/176843006/diff/1/src/types.cc#newcode274
src/types.cc:274: if (value->IsSmi()) return SignedSmall(region);
Shouldn't this be tagged?
https://codereview.chromium.org/176843006/diff/1/src/types.h
File src/types.h (right):
https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode103
src/types.h:103: // T->Is(SignedSmall())), and the to check whether a
specific case needs
nit: Seems to be a "latter" missing.
https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode172
src/types.h:172: V(Allocated, kReceiver | kFloat | kName)
\
Why kFloat here?
https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode174
src/types.h:174: V(Detectable, kDetectableReceiver | kFloat |
kName) \
Why kFloat here?
https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode175
src/types.h:175: V(NonNumber, kOddball | kAllocated |
kInternal) \
This way kNonNumber contains kFloat, this seems wrong.
https://codereview.chromium.org/176843006/diff/1/src/types.h#newcode321
src/types.h:321: enum PrintDim { BOTH_DIMS, SEMANTIC_DIM,
REPRESENTATION_DIM };
nit: Can we write out "PrintDimension" at least for the name of the enum
type (not the enum values)?
https://codereview.chromium.org/176843006/
--
--
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.
