search

gwtproject / gwt

GWT Open Source Project
http://www.gwtproject.org
1.52k stars 377 forks source link

StrictMath incorrectly implements atan2(y, x) using atan(x) #9754

Open tommyettinger opened 2 years ago

tommyettinger commented 2 years ago

GWT version: 2.10.0 Browser (with version): Found in source code of GWT Operating System: All

Description

StrictMath.atan2(double, double) incorrectly calls Math.atan(double) instead of Math.atan2(double, double). See https://github.com/gwtproject/gwt/blob/7de9ce8930d029cef35a41f79202213a736b9e86/user/super/com/google/gwt/emul/java/lang/StrictMath.java#L56-L58 for the one incorrect line.

Steps to reproduce

Call StrictMath.atan2() with almost any value for y other than 0. It's pretty clear this is a copy-paste error from the StrictMath.atan(double) code on the lines just above.

Known workarounds

Just use Math.atan2(double, double).

Links to further discussions
niloc132 commented 2 years ago

Thanks for the report - we're switching development to Github pull requests shortly (as soon as we've finished the release process for 2.10), would you be interested in proposing a patch with tests?

tommyettinger commented 2 years ago

My eyes glazed over reading the lengthy requirements to build in the README.md, so at this moment no, I am not at all interested in doing hours of work to change one line. I'm also quite suspicious of the need to install any closed-source proprietary toolchain to build open-source software. If the build system was reasonable, I would consider a patch for a larger or more complex change, but wrangling the build system seems quite unreasonable for small changes such as this. I could commit a change and test without building or running it, which seems somewhat silly for quality assurance purposes.

I've already typed more here than it would take to fix the issue, but here's the full changed code:

 public static double atan2(double y, double x) { 
   return Math.atan2(y, x); 
 }

Yes, atan2 takes y first, no, there's no convention across languages, so yeah, StrictMath does what Math does.

niloc132 commented 2 years ago

As above, the process is changing - pull requests are not open yet, but will be very soon. I don't know what closed source toolchain you are referring to - the build system is Apache Ant, and the tools repo is just a collection of jars so as to provide patched versions of open source libraries and tools. While Java 8 is presently required to build, this will be changing soon (and OpenJDK 8 builds are readily available).

Open source relies on lots of little bits of effort. I will definitely get to this bug (and a test for the rest of the class, apparently omitted in Google's original patch here?), but the effort I personally have to spend is spread thin.

Thanks for the report, we appreciate it.