Last Comment Bug 750580 - Remove cyclic dependency between Parser and BytecodeEmitter modules
: Remove cyclic dependency between Parser and BytecodeEmitter modules
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: 750282
Blocks: UntangleFrontEnd 750606
  Show dependency treegraph
 
Reported: 2012-04-30 18:10 PDT by Nicholas Nethercote [:njn]
Modified: 2012-05-10 14:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (72.31 KB, patch)
2012-04-30 18:10 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Review
patch v2 (71.45 KB, patch)
2012-05-03 23:53 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2012-04-30 18:10:43 PDT
Created attachment 619799 [details] [diff] [review]
patch

Currently, Parser.cpp includes BytecodeEmitter.h and BytecodeEmitter.cpp
includes Parser.h.  If you think of them as modules (e.g. Foo.h, Foo-inl.h
and Foo.cpp make up the Foo module), the dependency is this:

  Parser <--> BytecodeEmitter

BytecodeEmitter definitely needs to depend on Parser, but Parser shouldn't
need to depend on BytecodeEmitter.

By introducing a TreeContext module we can end up with this structure:

  TreeContext <-- Parser <-- BytecodeEmitter

This patch does this.  Specifically:

- It moves the TCF_* constants and TreeContext struct out of the
  BytecodeEmitter module into a new TreeContext module.

- It moves a bunch of other stuff (StmtType, StmtInfo, etc) that was in the
  BytecodeEmitter module but is needed by the Parser module into the Parser
  module.

- It moves SrcNoteLineScanner from jsopcodeinlines.h to
  BytecodeEmitter-inl.h, because the BytecodeEmitter module holds all the
  SrcNote stuff.

- It removes lots of now-unnecessary |#include "BytecodeEmitter.h"| lines.
  Some of them are replaced with |#include "TreeContext.h|.  (Note that
  TreeContext.h is a *much* smaller file than the old BytecodeEmitter.h.)
  Most notably, Parser.cpp no longer includes BytecodeEmitter.h.

Apologies for the hard-to-read patch, it mostly just moves code around.
Comment 1 Nicholas Nethercote [:njn] 2012-04-30 18:13:43 PDT
Note:  the new TreeContext-inl.h file doesn't show up in the Splinter review because it was created with a |hg copy| of BytecodeEmitter-inl.h.  You can see it in the raw diff.
Comment 2 Nicholas Nethercote [:njn] 2012-05-03 23:53:23 PDT
Created attachment 620979 [details] [diff] [review]
patch v2

Updated for the removal of DefineGlobal and BindTopLevelVar in bug 751818.  This patch applies on top of the patch in bug 750282.
Comment 3 :Ms2ger 2012-05-08 00:16:28 PDT
Comment on attachment 620979 [details] [diff] [review]
patch v2

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

::: js/src/frontend/TreeContext.h
@@ +14,5 @@
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is Mozilla Communicator client code, released
> + * March 31, 1998.

Heh. MPL2, please.
Comment 4 Jason Orendorff [:jorendorff] 2012-05-09 10:05:44 PDT
Comment on attachment 620979 [details] [diff] [review]
patch v2

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

Great. Just nits.

::: js/src/frontend/BytecodeEmitter-inl.h
@@ +45,4 @@
>  
>  namespace js {
>  
> +class SrcNoteLineScanner

Why move this? It operates on a finished JSScript, like the stuff in jsopcode.h/cpp, and it doesn't require access to the parser or any internals of the bytecode emitter. I think this code belongs in jsopcodeinlines.h.

::: js/src/frontend/Parser.cpp
@@ +7099,5 @@
>  }
> +
> +bool
> +frontend::SetStaticLevel(TreeContext *tc, unsigned staticLevel)
> +{

I'd like to see a file TreeContext.cpp for this stuff, instead of moving them to Parser.cpp, especially sense all these things basically should be TreeContext methods anyway. Any reason not to do that?
Comment 5 Nicholas Nethercote [:njn] 2012-05-09 18:54:20 PDT
Pushed with the suggested changes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/021f722e8022
Comment 6 Nicholas Nethercote [:njn] 2012-05-09 19:52:47 PDT
> Heh. MPL2, please.

I considered this, but every single other file in the JS engine is still listed as MPL 1.1, so I did likewise.  Updating them all should be a separate bug.
Comment 7 Ed Morley [:emorley] 2012-05-10 08:05:30 PDT
https://hg.mozilla.org/mozilla-central/rev/021f722e8022
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-10 14:50:55 PDT
(In reply to Nicholas Nethercote [:njn] from comment #6)
> I considered this, but every single other file in the JS engine is still
> listed as MPL 1.1, so I did likewise.  Updating them all should be a
> separate bug.

Maybe this is a case of deliberate rule violations, or the rules being overly dogmatic, or something, but the license policy says you must use MPL2 for new files:

http://www.mozilla.org/MPL/license-policy.html

I was using the tri-license myself on other code for a different reason, fully expecting the eventual world-changing patch would fix things up at that point, until I found out about this.

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