Daniel Kurka has posted comments on this change.

Change subject: adding compare for several number types -Byte.compare -Short.compare -Integer.compare -Long.compare -Float.compare (Double already exists) fixes issue 7998
......................................................................


Patch Set 9:

(4 comments)

....................................................
File user/super/com/google/gwt/emul/java/lang/Float.java
Line 37:     if (isNaN(x)) {
I don't think its a good idea, since with this we would have to do something like:

if(!isNaN(x) && !isNaN(y)) {
// do normal stuff
}else{
//test again
}

If we want to change this, we should also change this in Double.compare (its implemented the same way)


....................................................
File user/super/com/google/gwt/emul/java/lang/Short.java
Line 37:     return Integer.compare(x, y);
I don"t really understand what the difference would be between prod vs. dev if we do not use x-y. Correct me if I am wrong but one should never rely on the exact value returned from compare.


....................................................
File user/test/com/google/gwt/emultest/java/lang/FloatTest.java
Line 81:   }
In JavaScript something like this passes:
assertEquals(0, Float.compare(0.0f, -0.0f));

While it does not pass in Java. Do we want to make sure we cover this corner case (as well as others) and take the performance hit?

Not sure here. Could we just cover this in the docs of the JRE Emulation?


....................................................
File user/test/com/google/gwt/emultest/java/lang/IntegerTest.java
Line 120:     assertEquals(0, Integer.compare(1, 1));
We actually look terrible at the Borders:

This will just fail:
Integer.compare(Integer.MAX_VALUE,  Integer.MIN_VALUE) > 0

since we do not got overflow for ints in javascript.

I am not sure how to handle such corner cases, do we want to handle them, any input?


--
To view, visit https://gwt-review.googlesource.com/3180
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib33c93ff0fb3f7e4b93994a29d6e2a65898be246
Gerrit-PatchSet: 9
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Daniel Kurka <danku...@google.com>
Gerrit-Reviewer: Daniel Kurka <danku...@google.com>
Gerrit-Reviewer: Goktug Gokdogan <gok...@google.com>
Gerrit-Reviewer: John A. Tamplin <j...@jaet.org>
Gerrit-Reviewer: Leeroy Jenkins <jenk...@gwtproject.org>
Gerrit-Reviewer: Thomas Broyer <t.bro...@gmail.com>
Gerrit-HasComments: Yes

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--- You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.


Reply via email to