Closed Bug 1223652 Opened 4 years ago Closed 4 years ago

Remove else after return from CGBlockScopeList::findEnclosingScope.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: arai, Assigned: n.parkanyi, Mentored)

Details

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

Attachments

(1 file)

In CGBlockScopeList::findEnclosingScope, |else| clause is placed after |return| statement, we can remove the |else {| and corresponding |}| to reduce the indent of that block.

https://dxr.mozilla.org/mozilla-central/rev/cc473fe5dc512c450634506f68cbacfb40a06a23/js/src/frontend/BytecodeEmitter.cpp#8438
>         if (list[index].end == 0) {
>             // We are looking for the nearest enclosing live scope.  If the
>             // scope contains POS, it should still be open, so its length should
>             // be zero.
>             return list[index].index;
>         } else {
>             // Conversely, if the length is not zero, it should not contain
>             // POS.
>             MOZ_ASSERT_IF(inPrologue == list[index].endInPrologue, list[index].end <= pos);
>         }

Required code changes are following:
  * Remove else after return from CGBlockScopeList::findEnclosingScope above
  * reduce the indent of that block
  * re-wrap the comment there to fit to 79 chars

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 2 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
Mentor: arai.unmht
Whiteboard: [good first bug][lang=c++]
Comment on attachment 8686336 [details] [diff] [review]
Removes the redundant else block in CGBlockScopeList::findEnclosingScope.

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

Thank you for your patch :D
Excellent, it's perfect!

Here's a try run (it builds and runs automated tests) for your patch:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b65c241b944
After it runs without any error, your patch can be landed.

By the way, have you built SpiderMonkey (or Firefox) and tested your patch locally?
If not yet, it will be a good exercise, give it a try :)
  https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation#Building_SpiderMonkey
  https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation#Test
Attachment #8686336 - Flags: review?(arai.unmht) → review+
Assignee: nobody → n.parkanyi
Status: NEW → ASSIGNED
Try run is green :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bed0a37b59a6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.