Inline compareTo for numbers into min and max.

Inline compareTo for numbers into min and max.

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

[sra1, 2012-07-04 19:28:23]

DBC

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart
File corelib/src/math.dart (right):

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:63: if (a is num) {  // TODO(floitsch): move this into one
if.
Under what conditions? (description or issue #)
Presumably something undesirable happens if you do it now.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:66: // In the context of [min] we have furthermore -0.0 <
0.0.
What does 'in the context of [min]' mean here, in the body of [min]?

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:94: // In the context of [min] we have furthermore -0.0 <
0.0.
[max]

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:95: if (a < b) return a;
max(1,2) ==> 1  ?

make sure the test covers  A x A where A includes {NaN, -oo, -1, -1.0, -0.0,
0.0, 1.0, 1, +oo}

[Lasse Reichstein Nielsen, 2012-07-05 09:34:40]

LGTM, but try the short version and see if it's faster!

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart
File corelib/src/math.dart (right):

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:69: if (a is double && a == 0.0) {
This is a little tricky, since in dart2js 0.0 is both int and double.
If you wrote it as the, in pure Dart equivalent, 'a is! int' (since num has only
int and double as subclasses) it won't work. This might warrant a comment,.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:71: if (1.0 / a < 0.0) {
Consider checking as
  1.0 / a == -double.Infinity
instead. It's possible to do as a simple bit-wise comparison instead of doing
real computations in a non-finite value. That's usually very slow.

Or just replace this all with 
 if (a == b) {
   if (a == 0) { 
     // a and b are both either 0, 0.0 or -0.0
     // The following returns -0 if either a or b is -0,
     // and it returns an int if they are both int 0.
     return (a + b) * a * b;  
   }
   return a; 
 }

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:87: return a.compareTo(b) <= 0 ? a : b;
Consider using (0 >= a.compareTo(b)) to avoid having a compareTo return
something that isn't a number, but which has its own <= operator.
Never give a sucker an even break!

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:95: if (a < b) return a;
Yes, swap either the return or the comparison, but not both :)

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:97: if (a is double && a == 0.0) {
if (a == 0) return a + b;  // -0.0 if both are -0.0, otherwise 0.0 (or 0 if both
are int).

This does require that 0==0.0 and 0.0==-0.0 (and all the symmetric and
transitive consequences), but we should have that.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:113: }
Here you know that 'a' is num and 'b' is not, so you know what a.compareTo does.
(What does it do? throw?)

[floitsch, 2012-07-05 17:48:49]

Refactored the methods. Please have another look.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart
File corelib/src/math.dart (right):

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:63: if (a is num) {  // TODO(floitsch): move this into one
if.
On 2012/07/04 19:28:23, sra1 wrote:
> Under what conditions? (description or issue #)
> Presumably something undesirable happens if you do it now.

Done.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:66: // In the context of [min] we have furthermore -0.0 <
0.0.
On 2012/07/04 19:28:23, sra1 wrote:
> What does 'in the context of [min]' mean here, in the body of [min]?

Done.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:69: if (a is double && a == 0.0) {
On 2012/07/05 09:34:41, Lasse Reichstein Nielsen wrote:
> This is a little tricky, since in dart2js 0.0 is both int and double.
> If you wrote it as the, in pure Dart equivalent, 'a is! int' (since num has
only
> int and double as subclasses) it won't work. This might warrant a comment,.

removed with your code.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:71: if (1.0 / a < 0.0) {
On 2012/07/05 09:34:41, Lasse Reichstein Nielsen wrote:
> Consider checking as
>   1.0 / a == -double.Infinity
> instead. It's possible to do as a simple bit-wise comparison instead of doing
> real computations in a non-finite value. That's usually very slow.
> 
> Or just replace this all with 
>  if (a == b) {
>    if (a == 0) { 
>      // a and b are both either 0, 0.0 or -0.0
>      // The following returns -0 if either a or b is -0,
>      // and it returns an int if they are both int 0.
>      return (a + b) * a * b;  
>    }
>    return a; 
>  }

nice.
done.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:87: return a.compareTo(b) <= 0 ? a : b;
On 2012/07/05 09:34:41, Lasse Reichstein Nielsen wrote:
> Consider using (0 >= a.compareTo(b)) to avoid having a compareTo return
> something that isn't a number, but which has its own <= operator.
> Never give a sucker an even break!

Done.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:94: // In the context of [min] we have furthermore -0.0 <
0.0.
On 2012/07/04 19:28:23, sra1 wrote:
> [max]

Done.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:95: if (a < b) return a;
On 2012/07/04 19:28:23, sra1 wrote:
> max(1,2) ==> 1  ?
> 
> make sure the test covers  A x A where A includes {NaN, -oo, -1, -1.0, -0.0,
> 0.0, 1.0, 1, +oo}

Done.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:97: if (a is double && a == 0.0) {
On 2012/07/05 09:34:41, Lasse Reichstein Nielsen wrote:
> if (a == 0) return a + b;  // -0.0 if both are -0.0, otherwise 0.0 (or 0 if
both
> are int).
> 
> This does require that 0==0.0 and 0.0==-0.0 (and all the symmetric and
> transitive consequences), but we should have that.

done.

https://chromiumcodereview.appspot.com/10690086/diff/1/corelib/src/math.dart#...
corelib/src/math.dart:113: }
On 2012/07/05 09:34:41, Lasse Reichstein Nielsen wrote:
> Here you know that 'a' is num and 'b' is not, so you know what a.compareTo
does.
> (What does it do? throw?)

rewritten.

[Lasse Reichstein Nielsen, 2012-07-10 07:35:00]

There is also a math library in lib/math/base.dart that should also be updated.

LGTM.

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.dart
File corelib/src/math.dart (right):

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.da...
corelib/src/math.dart:62: static num min(num a, num b) {
Give it a doc-comment and say how it works (NaN if either value is NaN, -0.0 <
0.0, otherwise int/double with same mathematical value are equal). And that if
they are equal, you return the first argument (but I'd prefer documenting it as
being unspecified which one is returned).

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.da...
corelib/src/math.dart:77: return (a + b) * a * b;
This should work even if a or b are not doubles. At least you can remove the 'a
is double' check.
If you inserted it for performance, not correctness, do say so.

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.da...
corelib/src/math.dart:80: // Check for NaN and b == -0.0.
If you didn't check for "a is double" above, then b==NaN would be handled there.

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.da...
corelib/src/math.dart:109: // Check for NaN.
Don't we have b.isNaN()? I'd prefer using that, at least for readability.

[floitsch, 2012-07-10 17:32:32]

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.dart
File corelib/src/math.dart (right):

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.da...
corelib/src/math.dart:62: static num min(num a, num b) {
On 2012/07/10 07:35:00, Lasse Reichstein Nielsen wrote:
> Give it a doc-comment and say how it works (NaN if either value is NaN, -0.0 <
> 0.0, otherwise int/double with same mathematical value are equal). And that if
> they are equal, you return the first argument (but I'd prefer documenting it
as
> being unspecified which one is returned).

Done.

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.da...
corelib/src/math.dart:77: return (a + b) * a * b;
On 2012/07/10 07:35:00, Lasse Reichstein Nielsen wrote:
> This should work even if a or b are not doubles. At least you can remove the
'a
> is double' check.
> If you inserted it for performance, not correctness, do say so.

I want to return a consistent order when both arguments are equal. Without the
check min(0, 0.0) would return 0.0, but min(1, 1.0) would return 1.

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.da...
corelib/src/math.dart:80: // Check for NaN and b == -0.0.
On 2012/07/10 07:35:00, Lasse Reichstein Nielsen wrote:
> If you didn't check for "a is double" above, then b==NaN would be handled
there.
Yes, but then we wouldn't return the left argument.

https://chromiumcodereview.appspot.com/10690086/diff/6001/corelib/src/math.da...
corelib/src/math.dart:109: // Check for NaN.
On 2012/07/10 07:35:00, Lasse Reichstein Nielsen wrote:
> Don't we have b.isNaN()? I'd prefer using that, at least for readability.

Done.