Reduce indent in Parser<ParseHandler>::expr.

RESOLVED FIXED in Firefox 42

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: arai, Assigned: Xi Yang, Mentored)

Tracking

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Derived from bug 1066827 comment #43

http://hg.mozilla.org/mozilla-central/file/tip/js/src/frontend/Parser.cpp#l5746
>    if (matched) {
>        ....
>        return seq
>    }
>    return pn;

Invert above condition and reduce indent.
(Reporter)

Comment 1

3 years ago
I'm going to label this as [good first bug].  Required code change should be simple enough:
  * Reduce the indent of then-clause of "if (matched)" in Parser<ParseHandler>::expr method [1].
    Then-clause always returns, and code after the if is only "return pn;",
    so inverting the condition will reduce the entire indent.

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

A skilled first-time contributor should be able to finish this in two weeks.  Leave comments / questions here, or ask me (:arai) in IRC #introduction and #jsapi channels.
Note that I'm not a module peer, so I'll finally forward review request to appropriate reviewer :)

[1] http://hg.mozilla.org/mozilla-central/file/a1e66a69688f/js/src/frontend/Parser.cpp#l6296
(note that I linked to specific revision of the file to point the method, and it may be not latest)
[2] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
Mentor: arai.unmht
Whiteboard: [good first bug][lang=c++]
(Assignee)

Comment 2

3 years ago
Created attachment 8639003 [details] [diff] [review]
bug_1090695.patch
(Reporter)

Comment 3

3 years ago
Comment on attachment 8639003 [details] [diff] [review]
bug_1090695.patch

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

Thank you for your work :)
The code change looks perfect!

It would be nice if you can provide your name, e-mail, and commit message in your patch file, so the patch file become ready for checkin ;)
If you're using mercurial, try using `hg export` command, after setting the commit message (Bug 1090695 - Reduce indent in Parser<ParseHandler>::expr.) and configuring your .hgrc to generate a nice diff, by adding following 2 sections:


[ui]
username = your name <your e-mail address>

[diff]
git = 1
showfunc = 1
unified = 8


For more information, please refer following page (please ignore the "obsolete" mark on the page, we're going to make a new documentation)
https://developer.mozilla.org/en-US/docs/Archive/Mozilla/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F


Then, do you have try server access to test this patch?  if not, let me know (via bug comment or on IRC :arai), I'll push it there and post the URL for the test result here.
Attachment #8639003 - Flags: feedback+
(Assignee)

Comment 4

3 years ago
Created attachment 8639309 [details] [diff] [review]
bug_1090695.patch

Hi,

I've made the changes accordingly. Could you please help me to push to testing server? Thanks! :)
Attachment #8639003 - Attachment is obsolete: true
(Reporter)

Comment 5

3 years ago
Excellent! :D

Here's try push, it will take 2 hours or so.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bce37a47554e
(Reporter)

Comment 6

3 years ago
Comment on attachment 8639309 [details] [diff] [review]
bug_1090695.patch

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

All green on try run :)
It's ready for check in.

Thanks again for your patch!
Attachment #8639309 - Flags: review+
(Reporter)

Updated

3 years ago
Assignee: nobody → vinceyang15
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Assignee)

Comment 8

3 years ago
You are welcome. btw, is there anything else I can help with?

(In reply to Tooru Fujisawa [:arai] from comment #6)
> Comment on attachment 8639309 [details] [diff] [review]
> bug_1090695.patch
> 
> Review of attachment 8639309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All green on try run :)
> It's ready for check in.
> 
> Thanks again for your patch!
(Reporter)

Comment 9

3 years ago
(In reply to Xi Yang from comment #8)
> You are welcome. btw, is there anything else I can help with?

Yeah, there're a lot of things to do.  Have you checked other bugs in Bugs Ahoy?
  http://www.joshmatthews.net/bugsahoy/

If you're interested in JavaScript Engine, choose it in sidebar, there mentored bugs like this will be shown.  Besides, searching unassigned bugs on bugzilla would also be a nice way to find a bug to work on.
https://hg.mozilla.org/mozilla-central/rev/26d59ea97d2d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.