danno, thanks a lot for the review. It is much cleaner than patchset 1.
https://codereview.chromium.org/21014003/diff/1/build/features.gypi
File build/features.gypi (right):
https://codereview.chromium.org/21014003/diff/1/build/features.gypi#newcode44
build/features.gypi:44: 'v8_use_31_bits_smi_value%': 0,
On 2013/07/29 13:02:23, danno wrote:
Call these
v8_use_31_bit_smi_value
V8_USE_31_BIT_SMI_VALUE
Done.
https://codereview.chromium.org/21014003/diff/1/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/21014003/diff/1/include/v8.h#newcode5350
include/v8.h:5350: #if !V8_USE_31_BITS_SMI_VALUE
On 2013/07/29 13:02:23, danno wrote:
I also see no reason to put the SmiTagging<8> definition in a #ifdef
Also, reverse the sense of the #ifdef
#if V8_USE_31_BIT_SMI_VALUE
typedef SmiTagging<4> PlatformSmiTagging;
#else
typedef SmiTagging<kApiPointerSize> PlatformSmiTagging;
#endif
Done.
https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):
https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.cc#newcode3032
src/x64/assembler-x64.cc:3032: #if V8_USE_31_BITS_SMI_VALUE
On 2013/07/29 13:02:23, danno wrote:
No conditional #ifdefs in assembler
Done.
https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):
https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h#newcode382
src/x64/assembler-x64.h:382: explicit Immediate(Smi* value) {
On 2013/07/29 13:02:23, danno wrote:
Don't add a Smi* constructor here. The wrapper function that allows
inlining of
31-bit smis should do the work that this constructor does and call
through to
the existing version.
Done.
https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h#newcode996
src/x64/assembler-x64.h:996: #if V8_USE_31_BITS_SMI_VALUE
On 2013/07/29 13:02:23, danno wrote:
Is there some way to not including this conditionally? I think the
assembler
should have no conditionally included code, it should be as "dumb" as
possible
and not know anything about Smis if possible.
Done.
https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h#newcode1393
src/x64/assembler-x64.h:1393: void pcmpeqd(XMMRegister dst, XMMRegister
src);
On 2013/07/29 13:02:23, danno wrote:
Why can't this always be part of the code (no conditional include)?
Done.
https://codereview.chromium.org/21014003/diff/1/src/x64/disasm-x64.cc
File src/x64/disasm-x64.cc (right):
https://codereview.chromium.org/21014003/diff/1/src/x64/disasm-x64.cc#newcode1099
src/x64/disasm-x64.cc:1099: #endif
Removed the #ifdef
https://codereview.chromium.org/21014003/diff/1/src/x64/lithium-codegen-x64.h
File src/x64/lithium-codegen-x64.h (right):
https://codereview.chromium.org/21014003/diff/1/src/x64/lithium-codegen-x64.h#newcode125
src/x64/lithium-codegen-x64.h:125: void
DoDeferredNumberTagI(LNumberTagI* instr);
On 2013/07/29 13:02:23, danno wrote:
Please don't conditionally declare stuff unless it's absolutely
necessary, is
there any hurt to always having this thing present, even if it's not
used?
Done.
https://codereview.chromium.org/21014003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
