Closed Bug 1218204 Opened 4 years ago Closed 4 years ago

Remove else after return from Parser<ParseHandler>::maybeParseDirective.

Categories

(Core :: JavaScript Engine, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: arai, Assigned: bmanojkumar24, Mentored)

Details

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

Attachments

(1 file, 2 obsolete files)

In Parser<ParseHandler>::maybeParseDirective, |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/05d85d9bbce4660987a69574cc40b19157e42fea/js/src/frontend/Parser.cpp#3156
>                 if (pc->sc->isFunctionBox()) {
>                     // Request that this function be reparsed as strict.
>                     pc->newDirectives->setStrict();
>                     return false;
>                 } else {
>                     // We don't reparse global scopes, so we keep track of the
>                     // one possible strict violation that could occur in the
>                     // directive prologue -- octal escapes -- and complain now.
>                     if (tokenStream.sawOctalEscape()) {
>                         report(ParseError, false, null(), JSMSG_DEPRECATED_OCTAL);
>                         return false;
>                     }
>                     pc->sc->strictScript = true;
>                 }

Required code changes are following:
  * Remove else after return from Parser<ParseHandler>::maybeParseDirective 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
@arai I would like to work on this bug, can you please assign the bug to me :D. Thanks!!
oh nice :)
Assignee: nobody → bmanojkumar24
Attached patch Bug1218204.patch (obsolete) — Splinter Review
Please review my patch. Thanks :)
Attachment #8679447 - Flags: review?(arai.unmht)
Comment on attachment 8679447 [details] [diff] [review]
Bug1218204.patch

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

The change itself looks perfect.

The issue is the header.  I think it's not formatted correctly, and importing this patch results wrong commit message.
try following command to export the changeset

  hg export -r . > path_to_the_patch_file.patch
Attachment #8679447 - Flags: review?(arai.unmht) → feedback+
Attached patch Bug1218204.patch (obsolete) — Splinter Review
Please review the patch, I have used |hg export -r .| to create the patch.
Attachment #8679485 - Flags: review?(arai.unmht)
Comment on attachment 8679485 [details] [diff] [review]
Bug1218204.patch

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

the 3 lines from "user: ..." are also generated by |hg export| ?
Did you specify them in commit message?
ohh..yes.I specified them in the commit message..!
hmm, I'm not sure about the rule for 2nd line of commit message tho, at least the existence of user and branch there doesn't make sense.  user is stored in header and branch has no meaning.
can you remove those 3 lines?
Attached patch Bug1218204.patchSplinter Review
I have removed the three lines.
Attachment #8679447 - Attachment is obsolete: true
Attachment #8679485 - Attachment is obsolete: true
Attachment #8679485 - Flags: review?(arai.unmht)
Attachment #8679506 - Flags: review?(arai.unmht)
Comment on attachment 8679506 [details] [diff] [review]
Bug1218204.patch

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

Looks good, thank you! :)
Attachment #8679506 - Flags: review?(arai.unmht) → review+
Try run is green :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9f16e62baa3a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.