Remove "else" after return in ConvertScalar.

RESOLVED FIXED in Firefox 52

Status

()

defect
--
trivial
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: arai, Assigned: aaronjacob00, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

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

Comment 1

3 years ago
I made the requested changes. Please review.
Thanks.
Attachment #8793256 - Flags: review?(arai.unmht)
Reporter

Comment 2

3 years ago
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.
Attachment #8793256 - Flags: review?(arai.unmht) → feedback+
Assignee

Comment 3

3 years ago
Updated according to feedback. Please review.
Thanks.
Attachment #8793256 - Attachment is obsolete: true
Attachment #8793259 - Flags: review?(arai.unmht)
Reporter

Comment 4

3 years ago
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).
Attachment #8793259 - Flags: review?(arai.unmht) → review+
Assignee

Comment 6

3 years ago
(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?
Reporter

Comment 7

3 years ago
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.
Assignee: nobody → aaronjacob00
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 8

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9456e0185872
Remove else after return statement in ConvertScalar. r=arai
Keywords: checkin-needed

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9456e0185872
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.