plasma-umass / doppio

Breaks the browser language barrier (includes a plugin-free JVM).
http://plasma-umass.github.io/doppio-demo
MIT License
2.16k stars 175 forks source link

Implemented StrictMath.expm1 native method, added test check in Stric… #415

Closed hparuyr closed 8 years ago

hparuyr commented 8 years ago

I implemented StrictMath.expm1 native method during playing with doppio, also added as required by guide. Can this be small contribution from me?

Thanks, Paruyr

jvilk commented 8 years ago

Hey @hparuyr,

Thanks for your contribution! Just to check, you're not here due to Google Summer of Code, right? We've had an influx of users think they can start doing tasks with us for GSoC 2016, but GSoC 2016 hasn't been started yet, nor have we published any starter tasks for this year. (Regardless, I appreciate your PR!)

Could you actually define a polyfill for Math.expm1 in src/util.ts? There's a polyfill there for Math.imul; just put it right after that. Then, you can implement this native using your polyfill.

Also, is there any reason why you have two cases? It looks like MDN's polyfill for expm1 (linked above) is only:

function(x) {
  return Math.exp(x) - 1;
}

Was there an issue with the unit test not passing?

I'm on hand if you have any questions. Thanks!

jimfb commented 8 years ago

@jvilk For very small values of x, the expression Math.exp(x)-1 can be inaccurate, and d_val + 0.5*d_val*d_val; becomes a better approximation. For details, see: http://www.johndcook.com/blog/cpp_expm1/

@hparuyr Math.expm1() is defined natively as of ES6, and is supported by most browsers. I believe the native implementation (when available) gives even better accuracy than your enhanced polyfill (and also better performance). Does it make sense to have a check at the top that says if(Math.expm1 !== undefined) return Math.expm1(x);?

jvilk commented 8 years ago

@jimfb thanks for the clarification! I actually asked in my previous comment if he could make it a true polyfill, like I do for imul in src/util.ts.

jimfb commented 8 years ago

Ah, somehow I missed that sentence. Yes, your solution would be better than adding an if check at the top of the function :P.

hparuyr commented 8 years ago

Hi @jvilk,

Thank you for prompt replay. My contribution has nothing to do with "Google Summer of Code". I added polyfill to be consistent with rest of Math methods implementations already existing in StrictMath. I did no see any Math native method checks before polyfill, but agree it would be good practice to make it. I've made changes in src/util.ts and src/natives/java_lang.ts to use polyfill, if Math.expm1 native implementation is not available. Thanks to @jimfb also, to provide link and explanation about my polyfill. I can add that Math.exp(x)-1 is not passing the test.

jvilk commented 8 years ago

Excellent. Thanks for your contribution!

hparuyr commented 8 years ago

Thank you for accepting :)