Issue 24388004: Added bitLength, clamp(), toDouble(), toSigned(), toUnsigned().

Issue 24388004: Added bitLength, clamp(), toDouble(), toSigned(), toUnsigned(). (Closed)

Created:
7 years, 3 months ago by Chris Bracken

Modified:
7 years, 3 months ago

Reviewers:
sra1

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Added bitLength, clamp(), toDouble(), toSigned(), toUnsigned(). BUG= R=sra@google.com Committed: https://code.google.com/p/dart/source/detail?r=27862

Patch Set 3 : Use string literals in place of doubles where precision is exceeded #

Created: 7 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+386 lines, -15 lines)			Patch
M	pkg/fixnum/lib/src/int32.dart	View	1	2 chunks	+25 lines, -0 lines	0 comments	Download
M	pkg/fixnum/lib/src/int64.dart	View	1	4 chunks	+62 lines, -7 lines	0 comments	Download
M	pkg/fixnum/lib/src/intx.dart	View		3 chunks	+50 lines, -0 lines	0 comments	Download
M	pkg/fixnum/test/int_32_test.dart	View	1	3 chunks	+101 lines, -7 lines	0 comments	Download
M	pkg/fixnum/test/int_64_test.dart	View	1 2	4 chunks	+148 lines, -1 line	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

sra1

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int32.dart File pkg/fixnum/lib/src/int32.dart (right): https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int32.dart#newcode344 pkg/fixnum/lib/src/int32.dart:344: if (lowerLimit is int) return new Int32.fromInt(lowerLimit); fromInt is ...

7 years, 3 months ago (2013-09-24 02:58:04 UTC) #2

Chris Bracken

7 years, 3 months ago (2013-09-24 20:16:00 UTC) #3

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int32.dart
File pkg/fixnum/lib/src/int32.dart (right):

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int32.d...
pkg/fixnum/lib/src/int32.dart:344: if (lowerLimit is int) return new
Int32.fromInt(lowerLimit);
On 2013/09/24 02:58:04, sra1 wrote:
> fromInt is deprecated

Done.

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int64.dart
File pkg/fixnum/lib/src/int64.dart (right):

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int64.d...
pkg/fixnum/lib/src/int64.dart:523: a0 = ~a0 & _MASK;
On 2013/09/24 02:58:04, sra1 wrote:
> a0 = _MASK & a0
> 
> to help dart2js inference and consistency with most places.

Done.

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int64.d...
pkg/fixnum/lib/src/int64.dart:528: if (len > 0) return len + _BITS01;
On 2013/09/24 02:58:04, sra1 wrote:
> if a2 is nonzero, we don't use len.
> 
> How about
>   if (a2 != 0) return _BITS01 + a2.bitLength;
>   if (a1 != 0) return _BITS + a2.bitLength;
>   return a0.bitLength;

Done.

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int64.d...
pkg/fixnum/lib/src/int64.dart:552: if (this < lowerLimit) {
On 2013/09/24 02:58:04, sra1 wrote:
> Use the promoted values :-)

Done. Looks like I got two lines into cleaning this up then got distracted by
something!

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int64.d...
pkg/fixnum/lib/src/int64.dart:647: double toDouble() => toInt().toDouble();
On 2013/09/24 02:58:04, sra1 wrote:
> I think this can round incorrectly.
> 
> The problem lies in dart2js using doubles for the arithmetic on line 665 and
> 667.
> Doing:
>   big2 = (big1 + small1) + small2;
> can cause both small1 and small2 to be effectively ignored.
> We need to do
>   big2 = big1 + (small1 + small2)
> which can be written
>   big2 = small1 + small2 + big1;
> 

Done.

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int64.d...
pkg/fixnum/lib/src/int64.dart:663: result = (h << _BITS01) | (m << _BITS) | l;
On 2013/09/24 02:58:04, sra1 wrote:
> return negative ? -result - 1 : result;

Done.

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/lib/src/int64.d...
pkg/fixnum/lib/src/int64.dart:665: result = (h * 17592186044416) + (m * 4194304)
+ l;
On 2013/09/24 02:58:04, sra1 wrote:
> return negative ? -result - 1 : result;
> 
> But that has the problem we mention, so rewrite these two lines to
> 
>   if (negative) {
>     result = ((l + 1) + (m * 4194304)) + (h * 17592186044416);
>     return -result;
>   } else {
>     return (l + (m * 4194304)) + (h * 17592186044416);
>   }
>   
> 
> You should be able to find a value A near -(2^(53+1)) where you can see the
> difference.  On a single value, the rounding will seem arbitrary.
> 
> Take 100 random negative Int64s in this range (or 100 consecutive values).
>   A. convert to double, add.
>   B. add, then convert to double.
> 
> A will be closer to the correct answer B if the conversion does not have a
bias
> due to doing arithmetic with values that have already lost precision.
> I could have an off-by-one error in my description.
> 
> I think a batch of 9 specific value tests for
> -10000000000000000
> -10000000000000001
> -10000000000000002
> -10000000000000003
> -10000000000000004
> ...
> -10000000000000008
> should ensure that the exact VM and inexact dart2js produce the same values.

Done.

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/test/int_64_tes...
File pkg/fixnum/test/int_64_test.dart (right):

https://codereview.chromium.org/24388004/diff/3001/pkg/fixnum/test/int_64_tes...
pkg/fixnum/test/int_64_test.dart:544: expect(new Int64.fromInt(0).toDouble(),
same(0.0));
On 2013/09/24 02:58:04, sra1 wrote:
> fromInt is deprecated

Done.

sra1

LGTM with toDouble() test changes. https://codereview.chromium.org/24388004/diff/21001/pkg/fixnum/test/int_64_test.dart File pkg/fixnum/test/int_64_test.dart (right): https://codereview.chromium.org/24388004/diff/21001/pkg/fixnum/test/int_64_test.dart#newcode560 pkg/fixnum/test/int_64_test.dart:560: same(-10000000000000001.0)); Replace tabs with ...

7 years, 3 months ago (2013-09-24 21:56:34 UTC) #4

Chris Bracken

https://codereview.chromium.org/24388004/diff/21001/pkg/fixnum/test/int_64_test.dart File pkg/fixnum/test/int_64_test.dart (right): https://codereview.chromium.org/24388004/diff/21001/pkg/fixnum/test/int_64_test.dart#newcode560 pkg/fixnum/test/int_64_test.dart:560: same(-10000000000000001.0)); On 2013/09/24 21:56:34, sra1 wrote: > Replace tabs ...

7 years, 3 months ago (2013-09-24 23:59:37 UTC) #5

Chris Bracken

7 years, 3 months ago (2013-09-25 00:02:35 UTC) #6

Message was sent while issue was closed.

Committed patchset #3 manually as r27862 (presubmit successful).

Expand Messages | Collapse Messages