Make modInv throw Exception on incompatible operands.

Make modInv throw Exception on incompatible operands.

Also update errors to be more descriptive accross the integer methods.

I'm still considering whether a more precise "NotCoPrimeException" would be better.

R=regis@google.com, sgjesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/ede6cb71a5f6f974d2937aec3d2d319b179d0943

[Lasse Reichstein Nielsen, 2015-06-24 08:17:15]

lrn@google.com changed reviewers:
+ floitsch@google.com, sgjesse@google.com, srdjan@google.com

[Lasse Reichstein Nielsen, 2015-06-24 08:17:16]

lrn@google.com changed required reviewers:
+ sgjesse@google.com

[Søren Gjesse, 2015-06-24 08:24:12]

lgtm

[Lasse Reichstein Nielsen, 2015-06-24 08:47:08]

lrn@google.com changed reviewers:
+ regis@google.com

[regis, 2015-06-24 15:40:44]

lgtm with a few comments

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart
File runtime/lib/bigint.dart (right):

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart#new...
runtime/lib/bigint.dart:1400: if (e is! int) throw new ArgumentError.value(e,
"exponent");
Below, you added "not an integer"

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart#new...
runtime/lib/bigint.dart:1401: if (m is! int) throw new ArgumentError.value(m,
"modulus");
ditto

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart#new...
runtime/lib/bigint.dart:1562: throw new ArgumentError.value(0, "first operand",
"must not be zero");
x is 'this' at this point, i.e. the receiver for gcd.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart#new...
runtime/lib/bigint.dart:1565: throw new ArgumentError.value(0, "second operand",
"must not be zero");
y is the 'other' argument of gcd.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart
File runtime/lib/integers.dart (right):

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart#n...
runtime/lib/integers.dart:222: throw new RangeError.range(radix, 2, 36,
"radix");
To be consistent, we should throw an ArgumentError if radix is not an integer.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart#n...
runtime/lib/integers.dart:272: if (e is! int) throw new ArgumentError.value(e,
"exponent");
We should add "not an integer" as in other places.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart#n...
runtime/lib/integers.dart:273: if (m is! int) throw new ArgumentError.value(m,
"modulus");
ditto

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/js_lib/js_number.dart (right):

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/js_number.dart:393: if (e is! int) throw
argumentErrorValue(e);
Why not ArgumentError.value(e, "exponent", "not an integer") ?

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/js_number.dart:394: if (m is! int) throw
argumentErrorValue(m);
ditto

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/core/int.dart
File sdk/lib/core/int.dart (right):

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/core/int.dart#newco...
sdk/lib/core/int.dart:119: * Throws an [UnsupportedError] if no modular inverse
exists.
It is an error if ...

[Lasse Reichstein Nielsen, 2015-06-25 07:22:21]

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart
File runtime/lib/bigint.dart (right):

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart#new...
runtime/lib/bigint.dart:1562: throw new ArgumentError.value(0, "first operand",
"must not be zero");
I wasn't able to convince myself that this was always the case since _binaryGcd
is called from multiple places.
If it is, then it should just be `this` and `other`.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart
File runtime/lib/integers.dart (right):

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart#n...
runtime/lib/integers.dart:222: throw new RangeError.range(radix, 2, 36,
"radix");
True. 
Or maybe should just stop testing if the argument is an integer when it's
decared as "int radix".

If we just change it to (radix < 2 || 36 < radix) then that restricts it to
working with int and double, then the & in the next section will fail for
doubles, so it's not really needed.

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/js_lib/js_number.dart (right):

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/js_number.dart:393: if (e is! int) throw
argumentErrorValue(e);
I think the argumentErrorValue function might be there to deliberately not be
inlined. But not sure.
It could still take more arguments to give better errors.

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/core/int.dart
File sdk/lib/core/int.dart (right):

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/core/int.dart#newco...
sdk/lib/core/int.dart:119: * Throws an [UnsupportedError] if no modular inverse
exists.
As stated elsewhere, I think we might want to tell the user how they can detect
the case where no modular inverse exists.

On the other hand, I'm leaning more and more towards returning 0 in that case
and avoid throwing. So, let's iterate this some more. The current documentation
in 1.11 leaves us free to think some more.

[regis, 2015-06-25 16:51:46]

lgtm with comments

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart
File runtime/lib/integers.dart (right):

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart#n...
runtime/lib/integers.dart:222: throw new RangeError.range(radix, 2, 36,
"radix");
On 2015/06/25 07:22:21, Lasse Reichstein Nielsen wrote:
> True. 
> Or maybe should just stop testing if the argument is an integer when it's
> decared as "int radix".
> 
> If we just change it to (radix < 2 || 36 < radix) then that restricts it to
> working with int and double, then the & in the next section will fail for
> doubles, so it's not really needed.

Good point.

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/core/int.dart
File sdk/lib/core/int.dart (right):

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/core/int.dart#newco...
sdk/lib/core/int.dart:119: * Throws an [UnsupportedError] if no modular inverse
exists.
On 2015/06/25 07:22:21, Lasse Reichstein Nielsen wrote:
> As stated elsewhere, I think we might want to tell the user how they can
detect
> the case where no modular inverse exists.
> 
> On the other hand, I'm leaning more and more towards returning 0 in that case
> and avoid throwing. So, let's iterate this some more. The current
documentation
> in 1.11 leaves us free to think some more.

It was my thinking in the first version, where I was returning 0 in that case,
so the user does not have to catch an exception. However, Srdjan did not like it
and I then realized that 0 is a valid answer for a modulus of 1, so I gave in
and threw in that case.

I imagine that this function will be used for crypto to choose RSA keys. The
question is whether the user will use this function to iterate and find coprime
numbers (returning -1 would be better), or will verify with other means (gcd?)
that the operands are coprime (throwing an exception is then OK).

[sra1, 2015-06-25 21:28:47]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2015-06-25 21:28:48]

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart
File runtime/lib/integers.dart (right):

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart#n...
runtime/lib/integers.dart:222: throw new RangeError.range(radix, 2, 36,
"radix");
On 2015/06/25 16:51:45, regis wrote:
> On 2015/06/25 07:22:21, Lasse Reichstein Nielsen wrote:
> > True. 
> > Or maybe should just stop testing if the argument is an integer when it's
> > decared as "int radix".
> > 
> > If we just change it to (radix < 2 || 36 < radix) then that restricts it to
> > working with int and double, then the & in the next section will fail for
> > doubles, so it's not really needed.
> 
> Good point.

I have been writing this range check in the dart2js versions as

    2 <= radix && radix <= 36

It is a way of putting the constant in receiver position (which generates the
best JavaScript) without looking unnatural.

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/js_lib/js_number.dart (right):

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/js_number.dart:393: if (e is! int) throw
argumentErrorValue(e);
On 2015/06/25 07:22:21, Lasse Reichstein Nielsen wrote:
> I think the argumentErrorValue function might be there to deliberately not be
> inlined. But not sure.
> It could still take more arguments to give better errors.

If you want to pass more arguments to ArgumentError.value just use the
constructor.  It makes perfect sense to name the argument here since there is
more than one.

----

Don't add arguments to argumentErrorValue.

Dart2js compiles calls to static functions (and factory constructors) by
inserting the default constants. So

  new ArgumentError.value(x)

generates one of

  P.ArgumentError$value(x, null, null)       // call factory
  new P.ArgumentError(true, x, null, null)   // inlined factory -> allocation

or when minified, something like:

  P.xy(x,null,null)
  new P.uv(!0,x,null,null)

I added argumentErrorValue(x) for two reasons:

(1) to have a common function that is called from 'canned' optimized code and
the js_lib implementations to ensure that optimization does not change the
exception.  

(2) to have a static function that has only one argument to get rid of the nulls
everywhere in the js_lib code. It generates

  P.argumentErrorValue(x)
minified:
  P.jk(x)

argumentErrorValue is marked as NoInline to stop it being reexpanded to one of
the above expressions (though it should not in a throw expression, it is used in
other contexts in js_lib).

Ideally the compiler would be smart enough to do this tedious task for common
cold-code patterns but it is not.

[Lasse Reichstein Nielsen, 2015-06-30 05:50:48]

Addressed cmments. PTAL

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart
File runtime/lib/bigint.dart (right):

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart#new...
runtime/lib/bigint.dart:1400: if (e is! int) throw new ArgumentError.value(e,
"exponent");
On 2015/06/24 15:40:43, regis wrote:
> Below, you added "not an integer"

Done.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart#new...
runtime/lib/bigint.dart:1401: if (m is! int) throw new ArgumentError.value(m,
"modulus");
On 2015/06/24 15:40:43, regis wrote:
> ditto

Done.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/bigint.dart#new...
runtime/lib/bigint.dart:1565: throw new ArgumentError.value(0, "second operand",
"must not be zero");
On 2015/06/24 15:40:43, regis wrote:
> y is the 'other' argument of gcd.

Done.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart
File runtime/lib/integers.dart (right):

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart#n...
runtime/lib/integers.dart:222: throw new RangeError.range(radix, 2, 36,
"radix");
I'll go for (radix < 2 || 36 < radix) for the reverse test.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart#n...
runtime/lib/integers.dart:272: if (e is! int) throw new ArgumentError.value(e,
"exponent");
On 2015/06/24 15:40:43, regis wrote:
> We should add "not an integer" as in other places.

Done.

https://codereview.chromium.org/1209523002/diff/1/runtime/lib/integers.dart#n...
runtime/lib/integers.dart:273: if (m is! int) throw new ArgumentError.value(m,
"modulus");
Done.

If we simply removed the tests, what is the worst that could happen?
A double e would get caught at .isOdd, a double m would probably work all the
way through and give a meaningful result. Or wrong result if it goes to the
bigint case - but still, the world won't crash, and the analyzer should warn, so
it'll still be fine.

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
File sdk/lib/_internal/compiler/js_lib/js_number.dart (right):

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/js_number.dart:393: if (e is! int) throw
argumentErrorValue(e);
On 2015/06/25 21:28:48, sra1 wrote:
> On 2015/06/25 07:22:21, Lasse Reichstein Nielsen wrote:
> > I think the argumentErrorValue function might be there to deliberately not
be
> > inlined. But not sure.
> > It could still take more arguments to give better errors.
> 
> If you want to pass more arguments to ArgumentError.value just use the
> constructor.  It makes perfect sense to name the argument here since there is
> more than one.
> 
> ----
> 
> Don't add arguments to argumentErrorValue.
> 
> Dart2js compiles calls to static functions (and factory constructors) by
> inserting the default constants. So
> 
>   new ArgumentError.value(x)
> 
> generates one of
> 
>   P.ArgumentError$value(x, null, null)       // call factory
>   new P.ArgumentError(true, x, null, null)   // inlined factory -> allocation
> 
> or when minified, something like:
> 
>   P.xy(x,null,null)
>   new P.uv(!0,x,null,null)
> 
> I added argumentErrorValue(x) for two reasons:
> 
> (1) to have a common function that is called from 'canned' optimized code and
> the js_lib implementations to ensure that optimization does not change the
> exception.  
> 
> (2) to have a static function that has only one argument to get rid of the
nulls
> everywhere in the js_lib code. It generates
> 
>   P.argumentErrorValue(x)
> minified:
>   P.jk(x)
> 
> argumentErrorValue is marked as NoInline to stop it being reexpanded to one of
> the above expressions (though it should not in a throw expression, it is used
in
> other contexts in js_lib).
> 
> Ideally the compiler would be smart enough to do this tedious task for common
> cold-code patterns but it is not.

Done.

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/_internal/compiler/...
sdk/lib/_internal/compiler/js_lib/js_number.dart:394: if (m is! int) throw
argumentErrorValue(m);
On 2015/06/24 15:40:43, regis wrote:
> ditto

Done.

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/core/int.dart
File sdk/lib/core/int.dart (right):

https://codereview.chromium.org/1209523002/diff/1/sdk/lib/core/int.dart#newco...
sdk/lib/core/int.dart:119: * Throws an [UnsupportedError] if no modular inverse
exists.
I can see that 0 can be seen as a valid answer. After all, mod 0, most equality
requiements hold. Heck, 0 is even a multiplicative unit on all the elements :)

So -1, null or throw Exception or ArithmeticException seems to be the options.

[Søren Gjesse, 2015-06-30 08:18:25]

lgtm

[Lasse Reichstein Nielsen, 2015-06-30 12:46:31]

Committed patchset #4 (id:60001) manually as
ede6cb71a5f6f974d2937aec3d2d319b179d0943 (presubmit successful).

[regis, 2015-06-30 16:56:54]

lgtm

https://codereview.chromium.org/1209523002/diff/60001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/js_number.dart (right):

https://codereview.chromium.org/1209523002/diff/60001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_number.dart:512: throw new
ArgumentError.value(this, "first operand", "must not be zero");
'this'

https://codereview.chromium.org/1209523002/diff/60001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/js_number.dart:515: throw new
ArgumentError.value(this, "second operand", "must not be zero");
'other'