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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

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

Comment 1

[simplyblue24]

@arai I would like to work on this bug, can you please assign the bug to me :D. Thanks!!

Comment 2

[Tooru Fujisawa [:arai]]

oh nice :)

Comment 3

[simplyblue24]

Attachment: Bug1218204.patch

Please review my patch. Thanks :)

Comment 4

[Tooru Fujisawa [:arai]]

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

Comment 5

[simplyblue24]

Attachment: Bug1218204.patch

Please review the patch, I have used |hg export -r .| to create the patch.

Comment 6

[Tooru Fujisawa [:arai]]

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?

Comment 7

[simplyblue24]

ohh..yes.I specified them in the commit message..!

Comment 8

[Tooru Fujisawa [:arai]]

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?

Comment 9

[simplyblue24]

Attachment: Bug1218204.patch

I have removed the three lines.

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8679506 [details] [diff] [review]
Bug1218204.patch

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

Looks good, thank you! :)

Comment 11

[Tooru Fujisawa [:arai]]

Try run is running
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e4357ecc399

Comment 12

[Tooru Fujisawa [:arai]]

Try run is green :)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f16e62baa3a

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f16e62baa3a