Last Comment Bug 742612 - Reflect.parse: separate guarded/unguarded catch clauses
: Reflect.parse: separate guarded/unguarded catch clauses
Status: RESOLVED FIXED
reflect-parse
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Dave Herman [:dherman]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-04 20:10 PDT by Dave Herman [:dherman]
Modified: 2013-06-27 11:06 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first draft, no tests yet (5.23 KB, patch)
2012-08-19 08:45 PDT, Dave Herman [:dherman]
no flags Details | Diff | Splinter Review
second draft, now with tests (11.13 KB, patch)
2012-08-19 09:22 PDT, Dave Herman [:dherman]
jorendorff: review+
Details | Diff | Splinter Review

Description Dave Herman [:dherman] 2012-04-04 20:10:18 PDT
In bug 630897 we simplified the parser API to be a single array of catch clauses. But the API is getting popular independent of Firefox's implementation, so it makes sense to separate out the SpiderMonkey-specific parts of the API. I think this would be a better approach to catch clauses:

    interface TryStatement <: Statement {
        type: "TryStatement";
        block: BlockStatement;
        guardedHandlers: [ GuardedCatchClause ] | null;
        handler: CatchClause | null;
        finalizer: BlockStatement | null;
    }

    interface CatchClause <: Node {
        type: "CatchClause";
        param: Pattern;
        body: BlockStatement;
    }

    interface GuardedCatchClause <: Node {
        type: "CatchClause";
        param: Pattern;
        guard: Expression;
        body: BlockStatement;
    }

This separates out the SpiderMonkey-specific bits, because portable/standard JS code will only ever have a null .guardedHandlers property. It's also a good factoring for the SpiderMonkey grammar, because you always have to have at most one unguarded handler. So this is a more precisely-typed factoring.

I'll submit a patch as soon as I can.

Dave
Comment 1 Dave Herman [:dherman] 2012-04-04 20:13:08 PDT
BTW, the reason I prefer

    [ GuardedCatchClause ] | null

and not just

    [ GuardedCatchClause ]

is that it avoids allocating tons of empty arrays for all the cases where try statements have no guarded handlers.

Dave
Comment 2 Jason Orendorff [:jorendorff] 2012-04-06 09:10:02 PDT
Most scripts contain very few try blocks, if any. The extra arrays probably don't matter.
Comment 3 Dave Herman [:dherman] 2012-04-07 00:29:25 PDT
There's no reason to expect this API only to be used for hand-written web code. It can be used for any JS code anywhere, including code generated from other languages.

But I guess since the rest of the API uses empty arrays, it's bad form to treat this one case differently.

Dave
Comment 4 Ariya Hidayat 2012-04-20 15:05:28 PDT
Related Esprima issue: http://code.google.com/p/esprima/issues/detail?id=245.

Esprima is ready to change at a press of a button :)
Comment 5 Dave Herman [:dherman] 2012-04-22 00:10:25 PDT
> Esprima is ready to change at a press of a button :)

OK, let me do a bit of publicity before making the change.

It's going to miss the next Firefox deadline (4/24), so it won't get into Nightly for another 6 weeks, and then it's 6 weeks to Aurora, 6 weeks to Beta, and 6 weeks to Firefox. So if my math is right, I believe it'll most likely land in Firefox 16. I expect that should give the few existing customers enough time to prepare for the change, but we'll see.

Dave
Comment 6 Ariya Hidayat 2012-07-29 16:47:13 PDT
I'd like to release Esprima 1.0 in few weeks. Is it safe to assume that this will be the final API for TryStatement? I would prefer not to change Esprima's syntax tree format after it hits 1.0.
Comment 7 Dave Herman [:dherman] 2012-08-19 08:45:59 PDT
Created attachment 653188 [details] [diff] [review]
first draft, no tests yet

WIP. I'll work on tests next.

Dave
Comment 8 Dave Herman [:dherman] 2012-08-19 09:22:11 PDT
Created attachment 653190 [details] [diff] [review]
second draft, now with tests

Whoops, haha, let's try not to crash SpiderMonkey. The unguarded catch clause now defaults to null instead of leaking a magic hole to the builder.

Updated tests and added some for the builder.

Dave
Comment 9 Jason Orendorff [:jorendorff] 2012-08-22 12:31:19 PDT
Comment on attachment 653190 [details] [diff] [review]
second draft, now with tests

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

::: js/src/jsreflect.cpp
@@ +931,1 @@
>          return false;

Uber-micro-nit: While you're in here, I think this reads a little better if you common up the newArray call:

    Value guardedHandlers;
    if (!newArray(guarded, &guardedHandlers))
        return false;

    Value cb = callbacks[AST_TRY_STMT];
    if (!cb.isNull)
        return callback(...);

    return newNode(...);

@@ +2049,5 @@
>  {
>      Value var, guard, body;
>  
> +    if (!pattern(pn->pn_kid1, NULL, &var) ||
> +        !optExpression(pn->pn_kid2, &guard))

Style nit: js/src style rules call for curly braces when the if-condition spans multiple lines, and '{' on its own line (just in that case).

Alternatively you could just say

    if (!pattern(...))
        return false;
    if (!optExpression(...))
        return false;

Note You need to log in before you can comment on or make changes to this bug.