Closed Bug 1304310 Opened 4 years ago Closed 4 years ago

Remove "else" after return in ConvertScalar.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: arai, Assigned: aaronjacob00, Mentored)

Details

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

Attachments

(1 file, 1 obsolete file)

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
I made the requested changes. Please review.
Thanks.
Attachment #8793256 - Flags: review?(arai.unmht)
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+
Updated according to feedback. Please review.
Thanks.
Attachment #8793256 - Attachment is obsolete: true
Attachment #8793259 - Flags: review?(arai.unmht)
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+
(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?
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
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/9456e0185872
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.