1304310 - Remove "else" after return in ConvertScalar.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

https://dxr.mozilla.org/mozilla-central/rev/62f79d676e0e11b3ad59a5425b3ebb3ec5bbefb5/js/src/builtin/TypedObject.h#104
> template <typename T>
> static T ConvertScalar(double d)
> {
>     if (TypeIsFloatingPoint<T>()) {
>         return T(d);
>     } else if (TypeIsUnsigned<T>()) {
>         uint32_t n = JS::ToUint32(d);
>         return T(n);
>     } else {
>         int32_t n = JS::ToInt32(d);
>         return T(n);
>     }
> }

there, all branches always return, so "else" are just redundant and can be removed.

Required code changes are following:
  * Remove "else" after return from ConvertScalar above
  * reduce the indent of the last else block
  * remove the first braces, as SpiderMonkey's coding style omits braces for single-line block

You'll need basic knowledge of C++, and be able to build and test Firefox [1] or SpiderMonkey [2].

A skilled first-time contributor should be able to finish this in 1 weeks.  Leave comments / questions here, or ask me (:arai) in IRC #introduction and #jsapi channels.

[1] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

Comment 1

[Aaron]

Attachment: removed final 'else' statement, removed braces for first 'if' and applied appropriate indents

I made the requested changes. Please review.
Thanks.

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8793256 [details] [diff] [review]
removed final 'else' statement, removed braces for first 'if' and applied appropriate indents

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

Thank you for your patch :)

Please prepend "Bug 1304310 - " to commit message, and address following comments, and post it again.

::: js/src/builtin/TypedObject.h
@@ +107,2 @@
>          return T(d);
> +    else if (TypeIsUnsigned<T>()) {

here also "else" is unnecessary.
please remove it.

Comment 3

[Aaron]

Attachment: removed 'else' after return , removed braces for first 'if' and applied appropriate indents

Updated according to feedback. Please review.
Thanks.

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8793259 [details] [diff] [review]
removed 'else' after return , removed braces for first 'if' and applied appropriate indents

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

Perfect :)

I'll push your patch to try server (automation server for testing).

Comment 5

[Tooru Fujisawa [:arai]]

here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29330ce18218&selectedJob=27786190

Comment 6

[Aaron]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> here's try run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=29330ce18218&selectedJob=27786190

is there anything else i need to do?

Comment 7

[Tooru Fujisawa [:arai]]

after the test passes, you need to add checkin-needed keyword to this bug, so that the patch will be landed.
if you don't have permission (sorry I'm not sure who can modify keyword), ping me or ask someone in IRC to add the keyword.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9456e0185872
Remove else after return statement in ConvertScalar. r=arai

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9456e0185872