Move math functions to the js namespace

RESOLVED FIXED in mozilla34

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sankha, Assigned: arai)

Tracking

unspecified
mozilla34
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8464548 [details] [diff] [review]
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
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
(Assignee)

Comment 3

3 years ago
Created attachment 8465976 [details] [diff] [review]
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
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+
(Assignee)

Comment 5

3 years ago
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
(Assignee)

Comment 6

3 years ago
Created attachment 8466165 [details] [diff] [review]
Move math functions to the js namespace.

Oops, I wrote in wrong form...
Here is updated patch for comment 5.
Attachment #8465976 - Attachment is obsolete: true
Attachment #8466165 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/debc35b87cbb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/debc35b87cbb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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.