Don't throw exception for non-integer args to fixnum int32,int64 op ==

Don't throw exception for non-integer args to fixnum int32,int64 op ==

Original bug
https://code.google.com/p/dart/issues/detail?id=12073

BUG=
R=justinfagnani@google.com

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

[justinfagnani, 2013-07-26 23:44:40]

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

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int32.dart...
pkg/fixnum/lib/src/int32.dart:11: class int32 implements intx {
btw, I should have mentioned this last review, but I think these classes should
start with a capital "I".

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int32.dart...
pkg/fixnum/lib/src/int32.dart:143: int _toInt(val) {
all uses of this exception the one in ==() throw an argument error. how about
putting the throw here and just using custom logic in ==()?

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int32.dart...
pkg/fixnum/lib/src/int32.dart:168: return this.toInt64() + other;
for all these "this.toInt64()" calls, can you instead delegate to the RHS to
avoid creating a new object? Here you could have:

if (other is int64) {
  return other + this;
}

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

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int64.dart...
pkg/fixnum/lib/src/int64.dart:245: int64 _promote(val) {
Same as the int32 comment here, all but == throws an ArgumentError

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int64.dart...
pkg/fixnum/lib/src/int64.dart:392: int64 operator ~/(other) => _divMod(this,
_promote(other), false);
This will end up throwing a NoSuchMethodError (null pointer) in _divMode if
other in not int-ish, might as well throw the ArgumentError

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int64.dart...
pkg/fixnum/lib/src/int64.dart:1025: int64 _divMod(int64 a, int64 b, bool
computeRemainder) {
It looks like this is acting like a static method, even though it's an instance
method. I think the first argument should be removed in favor of using "this".

[Chris Bracken, 2013-07-27 01:07:11]

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

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int32.dart...
pkg/fixnum/lib/src/int32.dart:11: class int32 implements intx {
On 2013/07/26 23:44:40, justinfagnani wrote:
> btw, I should have mentioned this last review, but I think these classes
should
> start with a capital "I".

Agreed - will create a separate CL for this to avoid diff noise in this one.

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int32.dart...
pkg/fixnum/lib/src/int32.dart:143: int _toInt(val) {
On 2013/07/26 23:44:40, justinfagnani wrote:
> all uses of this exception the one in ==() throw an argument error. how about
> putting the throw here and just using custom logic in ==()?

Done.

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int32.dart...
pkg/fixnum/lib/src/int32.dart:168: return this.toInt64() + other;
On 2013/07/26 23:44:40, justinfagnani wrote:
> for all these "this.toInt64()" calls, can you instead delegate to the RHS to
> avoid creating a new object? Here you could have:
> 
> if (other is int64) {
>   return other + this;
> }

Done.

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int32.dart...
pkg/fixnum/lib/src/int32.dart:168: return this.toInt64() + other;
On 2013/07/26 23:44:40, justinfagnani wrote:
> for all these "this.toInt64()" calls, can you instead delegate to the RHS to
> avoid creating a new object? Here you could have:
> 
> if (other is int64) {
>   return other + this;
> }

As the code currently stands, it won't help. int64._promote() will do the same
thing. For the comparison, bit operators and addition I think we can make
changes to int64 that would take advantage of this. Will create a separate CL
for these.

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

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int64.dart...
pkg/fixnum/lib/src/int64.dart:245: int64 _promote(val) {
On 2013/07/26 23:44:40, justinfagnani wrote:
> Same as the int32 comment here, all but == throws an ArgumentError

Done.

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int64.dart...
pkg/fixnum/lib/src/int64.dart:392: int64 operator ~/(other) => _divMod(this,
_promote(other), false);
On 2013/07/26 23:44:40, justinfagnani wrote:
> This will end up throwing a NoSuchMethodError (null pointer) in _divMode if
> other in not int-ish, might as well throw the ArgumentError

Done.

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int64.dart...
pkg/fixnum/lib/src/int64.dart:1025: int64 _divMod(int64 a, int64 b, bool
computeRemainder) {
On 2013/07/26 23:44:40, justinfagnani wrote:
> It looks like this is acting like a static method, even though it's an
instance
> method. I think the first argument should be removed in favor of using "this".

It's used in toString() and toRadixString() with non-this values. I've made
_divMod() and _divModByShift() static for now.

[justinfagnani, 2013-07-27 01:23:54]

lgtm

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

https://codereview.chromium.org/20803006/diff/1/pkg/fixnum/lib/src/int64.dart...
pkg/fixnum/lib/src/int64.dart:1025: int64 _divMod(int64 a, int64 b, bool
computeRemainder) {
On 2013/07/27 01:07:11, Chris Bracken wrote:
> On 2013/07/26 23:44:40, justinfagnani wrote:
> > It looks like this is acting like a static method, even though it's an
> instance
> > method. I think the first argument should be removed in favor of using
"this".
> 
> It's used in toString() and toRadixString() with non-this values. I've made
> _divMod() and _divModByShift() static for now.

What I mean is that this method doesn't mix a and 'this'. Those calls can just
use a._divMod(...), rather than _divMod(a, ...)

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

https://codereview.chromium.org/20803006/diff/9001/pkg/fixnum/lib/src/int64.d...
pkg/fixnum/lib/src/int64.dart:779: // Defeat compile-time constant folding.
this worries me. if the compiler gets sufficiently smart, it'll evaluate this

[Chris Bracken, 2013-07-29 17:56:36]

Committed patchset #4 manually as r25577 (presubmit successful).