1034576 - Move math functions to the js namespace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sankha Narayan Guria [:sankha]]

When working on bug 1030699, bbouvier said that moving all jsmath functions to the js namespace is the right thing to do so that we can get rid of the "js_" prefix in the function names.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Move math functions to the js namespace.

Moved all functions in jsmath.h (except js_InitMathClass) to js namespace.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=c8e1b2b095b9

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8464548 [details] [diff] [review]
Move math functions to the js namespace.

Review of attachment 8464548 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing that!

::: js/src/jit/MCallOptimize.cpp
@@ +58,2 @@
>          return inlineMathSqrt(callInfo);
>      if (native == math_atan2)

while you're here, could you please write it js::math_atan2 here, for symmetry with the other cases?

::: js/src/jsmath.h
@@ +120,1 @@
>                     js::MutableHandleValue result);

nit: can you keep parameters alignment here? (does it fit in less than 100 chars now?)

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Move math functions to the js namespace.

Thank you for reviewing!

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> ::: js/src/jit/MCallOptimize.cpp
> @@ +58,2 @@
> >          return inlineMathSqrt(callInfo);
> >      if (native == math_atan2)
> 
> while you're here, could you please write it js::math_atan2 here, for
> symmetry with the other cases?

Okay, fixed.

> ::: js/src/jsmath.h
> @@ +120,1 @@
> >                     js::MutableHandleValue result);
> 
> nit: can you keep parameters alignment here? (does it fit in less than 100
> chars now?)

Oops, sorry I should be careful about alignment.
Fixed the alignment but the line is still over than 100 chars.


Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=08a0f8b386ac

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8465976 [details] [diff] [review]
Move math functions to the js namespace.

Review of attachment 8465976 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this! Sorry, I have one more suggestion, but it looks great. Feel free to upload a patch that addresses my comment and then auto-r+ it, try-server it (do you have L1 commit access?) and have it have its way to the build system :)

::: js/src/jsmath.cpp
@@ +110,5 @@
>          args.rval().setNaN();
>          return true;
>      }
>  
> +    return js::math_abs_handle(cx, args[0], args.rval());

sorry to nitpick: could you remove the js:: prefixing here please? and in all other changed function calls in this file.

@@ +735,4 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    return js::math_pow_handle(cx, args.get(0), args.get(1), args.rval());

here.

Comment 5

[Tooru Fujisawa [:arai]]

Thanks again!

Removed js:: prefixes in jsmath.cpp in previous patch and pre-existing one.
>    return js::math_round_handle(cx, args[0], args.rval());

Carrying forward bbouvier review in comment 4.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=8bb588dfbcb3

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Move math functions to the js namespace.

Oops, I wrote in wrong form...
Here is updated patch for comment 5.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/debc35b87cbb

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/debc35b87cbb