750580 - Remove cyclic dependency between Parser and BytecodeEmitter modules

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: 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 [inactive]]

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 [inactive]]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

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]]

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 [inactive]]

Pushed with the suggested changes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/021f722e8022

Comment 6

[Nicholas Nethercote [inactive]]

> 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]]

https://hg.mozilla.org/mozilla-central/rev/021f722e8022

Comment 8

[Jeff Walden [:Waldo]]

(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.