Closed Bug 1034576 Opened 7 years ago Closed 7 years ago

Move math functions to the js namespace

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: sankha, Assigned: arai)

Details

Attachments

(1 file, 2 obsolete files)

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.
Moved all functions in jsmath.h (except js_InitMathClass) to js namespace.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=c8e1b2b095b9
Attachment #8464548 - Flags: review?(benj)
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?)
Attachment #8464548 - Flags: review?(benj) → review+
Assignee: nobody → arai_a
Status: NEW → ASSIGNED
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
Attachment #8464548 - Attachment is obsolete: true
Attachment #8465976 - Flags: review?(benj)
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.
Attachment #8465976 - Flags: review?(benj) → review+
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
Oops, I wrote in wrong form...
Here is updated patch for comment 5.
Attachment #8465976 - Attachment is obsolete: true
Attachment #8466165 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/debc35b87cbb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.