Add 'fixnum' library to utils/

Add 'fixnum' library to utils/


BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=8804

[Dan Rice, 2012-05-14 21:34:44]

Hi -

  This adds a fixnum library to utils/ as we discussed.

Dan

[Mads Ager (google), 2012-05-15 07:37:39]

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/fixnum/int32.dart
File utils/fixnum/int32.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/fixnum/int32.dar...
utils/fixnum/int32.dart:333: List<int> result = new List<int>(8);
8 -> 4?

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/fixnum/int32.dar...
utils/fixnum/int32.dart:346: String toHexString() =>
_i.toRadixString(16).toUpperCase();
Why toUpperCase? This is only a matter of personal preference but I think we
should do what corelib does for toRadixString.

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/fixnum/int64.dart
File utils/fixnum/int64.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/fixnum/int64.dar...
utils/fixnum/int64.dart:15: // Note: several functions require _BITS == 22 -- do
not change this value
End comment with a period.

Also, I think it would be helpful if you added an overall comment about the
representation of a 64-bit integer here. I can deduce it from the below
constants but spelling it out might not be a bad idea.

I was looking for the fields actually representing the value. I expected to find
them either at the top or at the bottom of the file. Maybe we can move things
around to make it a bit easier to find fields, constructors, public methods,
internal methods?

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/fixnum/int64.dar...
utils/fixnum/int64.dart:581: return int32._numberOfLeadingZeros(_l) + 32;
Should we either make the computation explicit here as below of just use
constants below as well?

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
File utils/tests/fixnum/src/int32test.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
utils/tests/fixnum/src/int32test.dart:7: class Int32Test {
No need for the class.

Tests are no longer in a src directory and they use lower_case_test.dart names.

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
utils/tests/fixnum/src/int32test.dart:31: Expect.equals("223101104124", new
int32.fromInt(123456789).toRadixString(5));
Long lines.

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
File utils/tests/fixnum/src/int64VmTest.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
utils/tests/fixnum/src/int64VmTest.dart:1: 
Add copyright notice.

Tests are now using lower_case_test.dart names and this should as well.

Just call it int64_test? According to the status file this works on all
implementations?

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
utils/tests/fixnum/src/int64VmTest.dart:102: class int64VMTest {
int64Test? If this only works on the VM there should be something in the status
file?

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
utils/tests/fixnum/src/int64VmTest.dart:225: print("Test altered first argument
val0 = $val0, val0_save = $val0_save");
Long lines.

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
File utils/tests/fixnum/src/int64test.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
utils/tests/fixnum/src/int64test.dart:7: class Int64Test {
No need for the class. We could just use top-level functions?

Tests are no longer in a src directory and their names are lower_case_test.dart.

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/src...
utils/tests/fixnum/src/int64test.dart:182: Expect.equals(4503599627370495, new
int64.fromInt(4503599627370495).toInt());
Long lines.

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/tes...
File utils/tests/fixnum/test_config.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/1/utils/tests/fixnum/tes...
utils/tests/fixnum/test_config.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
No need for this file. Instead, just remove the src/ directory and use
lower_case_test.dart names and they will be run by test.dart.

[Mads Ager (google), 2012-05-15 07:42:07]

Shouldn't this be under lib/ as well?

[Dan Rice, 2012-06-12 17:18:15]

Moved everything under lib/ and got it working in dart2js

[Mads Ager (google), 2012-06-13 08:43:09]

Only a few real comments about the tests. Once those are addressed this looks
ready.

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.dart
File lib/fixnum/int64.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:495: res2 = a2 >> n; // was >>>
Why the comment here?

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:500: res1 = a2 >> (n - _BITS); // was >>>
Ditto.

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:505: res0 = a2 >> (n - _BITS01); // was >>>
Ditto.

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:525: int signa = _h >> (_BITS2 - 1);
_SIGN_BIT? Here and a couple of times below.

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:819: // work arout dart2js bugs with negative arguments to
>>
arout -> around

Would be nice to consistently start comments with a capital letter and end with
a period.

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:867: bool negative, bool aIsNegative, bool aIsMinValue,
Please use 4-space indent here so the arguments do not align with the body of
the method.

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
File tests/lib/fixnum/int_32_test.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_32_test.dart:9: print("int32 test");
Remove print.

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_32_test.dart:52: //  Expect.equals("-67153019", (new
int32.fromInt(123456789) *
Code in comments. Reenable test?

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
File tests/lib/fixnum/int_64_test.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_64_test.dart:9: print("int64 test");
Remove print?

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
File tests/lib/fixnum/int_64_vm_test.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_64_vm_test.dart:13: print("int64 vm test");
Remove?

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_64_vm_test.dart:38: testBase64();
Code in comments. Remove for now. It would make sense to add the tests for some
of these things that are in the interface but have not current tests. Maybe more
so in the int_64 and int_32 tests? That can be done as a follow up change.

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/lib.stat...
tests/lib/lib.status:5: fixnum/int_64_vm_test: Skip
This will skip this test for all compilers and runtimes. In that case there
seems to be no reason to add the test? 

Is this a test that should be run on the VM only? In that case we should add
this line below the "math/*: Skip" which skips the math tests for all
configurations that compile to JS but runs it in all configurations that use the
VM.

[Dan Rice, 2012-06-14 14:38:41]

Updated patch set uploaded

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.dart
File lib/fixnum/int64.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:495: res2 = a2 >> n; // was >>>
This was ported from a GWT Java code base.  It's O.K. to do a signed shift since
the incoming top bits are guaranteed to be 0.

On 2012/06/13 08:43:09, Mads Ager wrote:
> Why the comment here?

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:525: int signa = _h >> (_BITS2 - 1);
Changed to use '& _SIGN_BIT' instead of a shift.

On 2012/06/13 08:43:09, Mads Ager wrote:
> _SIGN_BIT? Here and a couple of times below.

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:819: // work arout dart2js bugs with negative arguments to
>>
Fixed.

On 2012/06/13 08:43:09, Mads Ager wrote:
> arout -> around
> 
> Would be nice to consistently start comments with a capital letter and end
with
> a period.

https://chromiumcodereview.appspot.com/10381133/diff/10001/lib/fixnum/int64.d...
lib/fixnum/int64.dart:867: bool negative, bool aIsNegative, bool aIsMinValue,
Fixed.

On 2012/06/13 08:43:09, Mads Ager wrote:
> Please use 4-space indent here so the arguments do not align with the body of
> the method.

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
File tests/lib/fixnum/int_32_test.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_32_test.dart:9: print("int32 test");
Fixed.

On 2012/06/13 08:43:09, Mads Ager wrote:
> Remove print.

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_32_test.dart:52: //  Expect.equals("-67153019", (new
int32.fromInt(123456789) *
Re-enabled.

On 2012/06/13 08:43:09, Mads Ager wrote:
> Code in comments. Reenable test?

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
File tests/lib/fixnum/int_64_test.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_64_test.dart:9: print("int64 test");
Fixed.

On 2012/06/13 08:43:09, Mads Ager wrote:
> Remove print?

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
File tests/lib/fixnum/int_64_vm_test.dart (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_64_vm_test.dart:13: print("int64 vm test");
Fixed.

On 2012/06/13 08:43:09, Mads Ager wrote:
> Remove?

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/fixnum/i...
tests/lib/fixnum/int_64_vm_test.dart:38: testBase64();
Removed.

On 2012/06/13 08:43:09, Mads Ager wrote:
> Code in comments. Remove for now. It would make sense to add the tests for
some
> of these things that are in the interface but have not current tests. Maybe
more
> so in the int_64 and int_32 tests? That can be done as a follow up change.

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://chromiumcodereview.appspot.com/10381133/diff/10001/tests/lib/lib.stat...
tests/lib/lib.status:5: fixnum/int_64_vm_test: Skip
The test works and I believe it has value, but it runs too slowly to be part of
the automated test framework.  I'll add a comment about it.

On 2012/06/13 08:43:09, Mads Ager wrote:
> This will skip this test for all compilers and runtimes. In that case there
> seems to be no reason to add the test? 
> 
> Is this a test that should be run on the VM only? In that case we should add
> this line below the "math/*: Skip" which skips the math tests for all
> configurations that compile to JS but runs it in all configurations that use
the
> VM.

[Mads Ager (google), 2012-06-14 14:57:22]

LGTM!

https://chromiumcodereview.appspot.com/10381133/diff/18001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://chromiumcodereview.appspot.com/10381133/diff/18001/tests/lib/lib.stat...
tests/lib/lib.status:10: fixnum/int_64_vm_test: Skip
Thanks for adding the comment. Does it make sense (in a follow-up change) to
scale down the test with fewer cases and always run it on the VM? Then the
manual step could be to change something in the test file itself to go through
the full set of tests?