Last Comment Bug 752793 - Remove Parser-only flags from TreeContextFlags
: Remove Parser-only flags from TreeContextFlags
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 752758
Blocks: UntangleFrontEnd 752816
  Show dependency treegraph
 
Reported: 2012-05-07 20:52 PDT by Nicholas Nethercote [:njn]
Modified: 2012-05-15 06:32 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: remove TCF_RETURN_{EXPR,VOID} (14.25 KB, patch)
2012-05-07 20:54 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review
patch 2: remove TCF_HAS_FUNCTION_STMT (10.78 KB, patch)
2012-05-07 20:55 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review
patch 3: remove TCF_DECL_DESTRUCTURING (6.45 KB, patch)
2012-05-07 20:55 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
patch 3, v2: remove TCF_DECL_DESTRUCTURING (12.20 KB, patch)
2012-05-07 21:10 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2012-05-07 20:52:32 PDT
In this bug I will remove the flags in TreeContextFlags that only relate to parsing, and find nicer ways to deal with them, i.e. so they're not exposed to every corner of the front-end.
Comment 1 Nicholas Nethercote [:njn] 2012-05-07 20:54:49 PDT
Created attachment 621865 [details] [diff] [review]
patch 1: remove TCF_RETURN_{EXPR,VOID}

TCF_RETURN_{EXPR,VOID} are only used in the parser, for issuing
warnings/errors.  This patch removes them from TreeContextFlags (where they
can be seen by BytecodeEmitter!) and puts them in TreeContext as single-bit
fields.

Notes:

- They're initialized to false when a new TreeContext is created for a
  function.

- They're set when a return statement of the appropriate kind is seen.

- They're never cleared.  Therefore the presence of TCF_RETURN_FLAGS in the
  assignment in Parser::statement() isn't necessary.

- They're checked in a couple of places.

- In LeaveFunction(), |funbox->tcflags| *doesn't* need to be assigned them
  -- it's only used for communicating state to the BytecodeEmitter, and
  BytecodeEmitter doesn't use TCF_RETURN_{EXPR,VOID}.
Comment 2 Nicholas Nethercote [:njn] 2012-05-07 20:55:27 PDT
Created attachment 621866 [details] [diff] [review]
patch 2: remove TCF_HAS_FUNCTION_STMT

TCF_HAS_FUNCTION_STMT is also only used in the parser, and its use is so
local that it can be replaced with an outparam in statements().  This is
much easier to understand than the flag;  the old code had to worry about
setting and clearing the flag for nested invocations of statements().
Comment 3 Nicholas Nethercote [:njn] 2012-05-07 20:55:59 PDT
Created attachment 621867 [details] [diff] [review]
patch 3: remove TCF_DECL_DESTRUCTURING

TCF_DECL_DESTRUCTURING is easy -- it's set and then cleared for certain
contexts.  It's easily moved into a bit-field in TreeContext.
Comment 4 Nicholas Nethercote [:njn] 2012-05-07 21:10:54 PDT
Created attachment 621870 [details] [diff] [review]
patch 3, v2: remove TCF_DECL_DESTRUCTURING

The previous patch was missing a piece.
Comment 5 Luke Wagner [:luke] 2012-05-09 17:08:00 PDT
Comment on attachment 621865 [details] [diff] [review]
patch 1: remove TCF_RETURN_{EXPR,VOID}

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

::: js/src/frontend/Parser.cpp
@@ +2692,5 @@
>  #endif
> +            tc->hasReturnVoid = true;
> +    }
> +
> +    if (tc->hasReturnExpr && (tc->sc->flags & TCF_FUN_IS_GENERATOR)) {

Aw, but it was so readable before.
Comment 6 Nicholas Nethercote [:njn] 2012-05-09 17:41:30 PDT
> Aw, but it was so readable before.

Tell me about it -- I had to draw a truth table to check that I was understanding the old version correctly.

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