Introduce v8::Integer::NewFromUnsigned method.

Introduce v8::Integer::NewFromUnsigned method.


Committed: http://code.google.com/p/v8/source/detail?r=3038

[antonm, 2009-10-06 07:05:03]

And another try.

I ran v8 tests in all combinations (6) + built chromium on win, mac and linux +
ran through buildbots.

[Lasse Reichstein, 2009-10-06 07:43:20]

LGTM

http://codereview.chromium.org/251100/diff/10/1002
File include/v8.h (right):

http://codereview.chromium.org/251100/diff/10/1002#newcode3030
Line 3030: Local<Integer> Integer::NewFromUnsigned(uint32_t value) {
Move this to api.cc (to be with Integer::New(int32_t)).

http://codereview.chromium.org/251100/diff/10/1002#newcode3033
Line 3033: if (could_be_smi) {
To check if something can be a Smi, use Smi::IsValid().
However, I see that there is no version of that with an unsigend argument, which
means that one should be carefull to avoid automatic casts from unsigned to
sigend, so alternatively use:
 if (value <= static_cast<uint32_t>(Smi::kMaxValue)) { ...

http://codereview.chromium.org/251100/diff/10/1004
File src/checks.h (right):

http://codereview.chromium.org/251100/diff/10/1004#newcode100
Line 100: static_cast<uint32_t>(value));
Nice hack :)

http://codereview.chromium.org/251100/diff/10/1001
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/251100/diff/10/1001#newcode715
Line 715: int32_t value = (1 << 30) - 1;
If you want (Smi::kMaxValue - 1), write it directly. Then it will work even if
the precission of smis change.
(If the precission ever becomes more than 32 bits, we'll worry about that
problem then).

http://codereview.chromium.org/251100/diff/10/1001#newcode726
Line 726: int32_t value = (1 << 30) + 1;
This one can't be converted to (Smi::kMaxValue + 1), since that operation might
overflow if smis have 32 bits of precission. However, that only happens on
64-bit, so you might change int32_t to intptr_t and avoid the overflow.

http://codereview.chromium.org/251100/diff/10/1001#newcode766
Line 766: uint32_t value = uint32_t(0xffffffff);
Just write 0xffffffffu for an unsigned value (don't use C-style casts).

[antonm, 2009-10-06 12:26:07]

thanks a lot for comments, Lasse.

http://codereview.chromium.org/251100/diff/10/1002
File include/v8.h (right):

http://codereview.chromium.org/251100/diff/10/1002#newcode3030
Line 3030: Local<Integer> Integer::NewFromUnsigned(uint32_t value) {
On 2009/10/06 07:43:20, Lasse Reichstein wrote:
> Move this to api.cc (to be with Integer::New(int32_t)).

if you please, no---I'm planning to move Integer::New into header, not vice
versa---those are tiny methods (on fast path).

http://codereview.chromium.org/251100/diff/10/1002#newcode3033
Line 3033: if (could_be_smi) {
On 2009/10/06 07:43:20, Lasse Reichstein wrote:
> To check if something can be a Smi, use Smi::IsValid().
> However, I see that there is no version of that with an unsigend argument,
which
> means that one should be carefull to avoid automatic casts from unsigned to
> sigend, so alternatively use:
>  if (value <= static_cast<uint32_t>(Smi::kMaxValue)) { ...
> 

Lasse, I've got a trouble here: I don't want to copy the constant from
src/objects.h and still wish to keep this function in header.  Will it be fine
to use v8::internal::Smi::kMaxValue in unit tests (as you suggest anyway) and
just hardcode this value here?

Doing it for now, but waiting for your opinion.

Another option---I don't care :)  Actually if value fits into int32_t, I should
use Integer::New anyway.  Renamed for clarity.

What would you say?

http://codereview.chromium.org/251100/diff/10/1004
File src/checks.h (right):

http://codereview.chromium.org/251100/diff/10/1004#newcode100
Line 100: static_cast<uint32_t>(value));
On 2009/10/06 07:43:20, Lasse Reichstein wrote:
> Nice hack :)

I'd better have PRI.. on all the platforms :)

http://codereview.chromium.org/251100/diff/10/1001
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/251100/diff/10/1001#newcode715
Line 715: int32_t value = (1 << 30) - 1;
On 2009/10/06 07:43:20, Lasse Reichstein wrote:
> If you want (Smi::kMaxValue - 1), write it directly. Then it will work even if
> the precission of smis change.
> (If the precission ever becomes more than 32 bits, we'll worry about that
> problem then).

Very good point, thanks.  Done.

http://codereview.chromium.org/251100/diff/10/1001#newcode726
Line 726: int32_t value = (1 << 30) + 1;
On 2009/10/06 07:43:20, Lasse Reichstein wrote:
> This one can't be converted to (Smi::kMaxValue + 1), since that operation
might
> overflow if smis have 32 bits of precission. However, that only happens on
> 64-bit, so you might change int32_t to intptr_t and avoid the overflow.

But I won't be able to pass it into v8::Integer::New then?  What about
conditional execution?

http://codereview.chromium.org/251100/diff/10/1001#newcode766
Line 766: uint32_t value = uint32_t(0xffffffff);
On 2009/10/06 07:43:20, Lasse Reichstein wrote:
> Just write 0xffffffffu for an unsigned value (don't use C-style casts).

Sorry, forgot to adjust this place.  Done.

[Lasse Reichstein, 2009-10-07 08:56:37]

http://codereview.chromium.org/251100/diff/10/1002
File include/v8.h (right):

http://codereview.chromium.org/251100/diff/10/1002#newcode3030
Line 3030: Local<Integer> Integer::NewFromUnsigned(uint32_t value) {
That would work too. It was the inconsistency that nagged me.

http://codereview.chromium.org/251100/diff/10/1002#newcode3033
Line 3033: if (could_be_smi) {
You could move Smi::kMaxValue (and kMinValue as well) to this file, as
kSmiMaxValue.
This is the file that defines kSmiTagSize and will (when I commit 32-bit smis
for x64) define the range of smis, so it is a fitting place for the definition.
You could even define a ValidSmiValue function here, in the internal namespace,
and use that in Smi::IsValid.

I think that's what I would prefer.

Whatever you do, you'll have to do the same to Integer::New if you move it here.

http://codereview.chromium.org/251100/diff/10/1001
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/251100/diff/10/1001#newcode726
Line 726: int32_t value = (1 << 30) + 1;
For now, you could add an ASSERT that Smi::kMaxValue is less than the maximal
value of int32_t.
When someone (read: me) changes the range of smis, I'll have to find a way
around it (e.g., conditional execution), but I'll also have introduced the
variables I need to test.

[William Hesse, 2009-10-07 14:30:33]

LGTM, if the two comments below are addressed.

http://codereview.chromium.org/251100/diff/3002/3006
File src/checks.h (right):

http://codereview.chromium.org/251100/diff/3002/3006#newcode84
Line 84: // arguments.  Should not be called directly.
This overload seems as dangerous as the unsigned one.  Have you checked all
platforms and compilers to make sure it doesn't cause problems?  I think I just
removed one exactly like it after it caused problems.  CHECK_EQ( int, int_64_t )
will be ambiguous, I think, won't it?

http://codereview.chromium.org/251100/diff/3002/3003
File test/cctest/test-api.cc (left):

http://codereview.chromium.org/251100/diff/3002/3003#oldcode28
Line 28: #include <stdlib.h>
Is this an indication that anyone embedding V8 will have a problem if they
include both stdlib and v8.h?  That would be a problem, wouldn't it?

[antonm, 2009-10-07 15:09:33]

Thanks a lot for another round, Lasse.

http://codereview.chromium.org/251100/diff/10/1002
File include/v8.h (right):

http://codereview.chromium.org/251100/diff/10/1002#newcode3033
Line 3033: if (could_be_smi) {
On 2009/10/07 08:56:37, Lasse Reichstein wrote:
> You could move Smi::kMaxValue (and kMinValue as well) to this file, as
> kSmiMaxValue.
> This is the file that defines kSmiTagSize and will (when I commit 32-bit smis
> for x64) define the range of smis, so it is a fitting place for the
definition.
> You could even define a ValidSmiValue function here, in the internal
namespace,
> and use that in Smi::IsValid.
> 
> I think that's what I would prefer.
> 
> Whatever you do, you'll have to do the same to Integer::New if you move it
here.

Lasse, I do understand necessity of moving Smi::kMaxValue here.  However in this
particular case I think int32'ness check is more modular---now unsigned
shouldn't know about the details of Integer:New, it just passes a value into it
if it could fit into int32_t.

http://codereview.chromium.org/251100/diff/10/1001
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/251100/diff/10/1001#newcode726
Line 726: int32_t value = (1 << 30) + 1;
On 2009/10/07 08:56:37, Lasse Reichstein wrote:
> For now, you could add an ASSERT that Smi::kMaxValue is less than the maximal
> value of int32_t.
> When someone (read: me) changes the range of smis, I'll have to find a way
> around it (e.g., conditional execution), but I'll also have introduced the
> variables I need to test.
> 

Ok, reverting to ASSERTs for now.

[antonm, 2009-10-07 15:13:01]

Thanks a lot for comments, William.

http://codereview.chromium.org/251100/diff/3002/3006
File src/checks.h (right):

http://codereview.chromium.org/251100/diff/3002/3006#newcode84
Line 84: // arguments.  Should not be called directly.
On 2009/10/07 14:30:33, William Hesse wrote:
> This overload seems as dangerous as the unsigned one.  Have you checked all
> platforms and compilers to make sure it doesn't cause problems?  I think I
just
> removed one exactly like it after it caused problems.  CHECK_EQ( int, int_64_t
)
> will be ambiguous, I think, won't it?

It builds fine as far as I can tell.  int64_t is defined to be long int actually
(thus ambiguities with double).  If you'd like, I can make it CHECK_EQ_INT_64 as
you did, but I didn't want to make it a special case unless really needed.

http://codereview.chromium.org/251100/diff/3002/3003
File test/cctest/test-api.cc (left):

http://codereview.chromium.org/251100/diff/3002/3003#oldcode28
Line 28: #include <stdlib.h>
On 2009/10/07 14:30:33, William Hesse wrote:
> Is this an indication that anyone embedding V8 will have a problem if they
> include both stdlib and v8.h?  That would be a problem, wouldn't it?

Nope as far as know---I just cleaned includes a bit (we have some unnessary
includes, apparently)

[Lasse Reichstein, 2009-10-08 07:37:50]

http://codereview.chromium.org/251100/diff/10/1002
File include/v8.h (right):

http://codereview.chromium.org/251100/diff/10/1002#newcode3033
Line 3033: if (could_be_smi) {
That sounds perfectly reasonable too.