Søren, thanks a lot for the review! All issues addressed. Please take
another
look.
-- Vitaly
http://codereview.chromium.org/1539039/diff/4001/5001
File src/globals.h (right):
http://codereview.chromium.org/1539039/diff/4001/5001#newcode222
src/globals.h:222: const int32_t kInt32SignMask = 0x80000000;
On 2010/04/16 08:38:33, Søren Gjesse wrote:
type uint32_t instead?
Our similar constants in include/v8.h are of type intptr_t and it turned
out we already have the sign mask constant for intptr_t. Since this mask
happens to also work for smis I reused it in the definition of
kSmiSignMask.
http://codereview.chromium.org/1539039/diff/4001/5002
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/1539039/diff/4001/5002#newcode12491
src/ia32/codegen-ia32.cc:12491: __ test(index, Immediate(kSmiTagMask |
kInt32SignMask));
On 2010/04/16 08:38:33, Søren Gjesse wrote:
kInt32SignMask -> kSmiSignMask?
Also (kSmiTagMask | 0x80000000) is used in quite a few places, please
change
these as well (if dealing with checking the sign).
Done. Changed all ia32 places I could find where I was sure the sign is
being tested.
http://codereview.chromium.org/1539039/diff/4001/5002#newcode12533
src/ia32/codegen-ia32.cc:12533: // Check that the right hand side is the
empty string (ie if this is really a
On 2010/04/16 08:38:33, Søren Gjesse wrote:
Check that -> Check if/Check whether?
Done.
http://codereview.chromium.org/1539039/diff/4001/5002#newcode12542
src/ia32/codegen-ia32.cc:12542: __ movzx_b(result, FieldOperand(result,
Map::kInstanceTypeOffset));
On 2010/04/16 08:38:33, Søren Gjesse wrote:
In this case we know that the string is either a flat or an external
string. I
am not sure whether it makes sense to use that information.
I'm not sure I follow you here. Do you mean we know this about the first
component of the cons string? I think we can still have e.g. a cons
string there.
http://codereview.chromium.org/1539039/diff/4001/5003
File src/ia32/codegen-ia32.h (right):
http://codereview.chromium.org/1539039/diff/4001/5003#newcode891
src/ia32/codegen-ia32.h:891: // overhead. Copying of overlapping regions
is not supported.
On 2010/04/16 07:18:03, Søren Gjesse wrote:
Drive by comment,
If we will be using these code generation methods from other places
than classes
inheriting from StringStubBase then I suggest getting rid of the
StringStubBase
class and move the functions to a StringHelper class inside
codegen-ia32.cc
(like the FloatingPointHelper class).
Done. StringHelper is now a simple namespace with helper functions. We
still need it in the header file to allow calls from ic-ia32.cc.
http://codereview.chromium.org/1539039/diff/4001/5003#newcode931
src/ia32/codegen-ia32.h:931: // Otherwise, scratch and result clobbered.
On 2010/04/16 08:38:33, Søren Gjesse wrote:
result clobbered -> result are clobbered
Done.
http://codereview.chromium.org/1539039/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
