1460489 - Move BytecodeEmitter's related classes into separate file and expose declaration via header file

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

if BytecodeEmitter consumer is going to use helper classes (IfEmitter, TryEmitter, etc) directly, they should be exposed via header file.
it would be nice to split them into a new cpp file.
and also related classes (TDZCheckCache, NestableControl and derived classes) should also be moved to that cpp file.

bug 1456006, bug 1456404, and all other classes should also be added into the cpp file.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move TDZCheckCache to BCEHelper.{cpp.h}.

Given that we're going to add more helper classes for emitting statements/expressions etc, moved existing helpers and related classes into separate cpp/h files.

Part 1 moves TDZCheckCache to BCEHelper.{cpp.h}.
(Let me know if you have better filename for storing all those classes)

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Move NestableControl classes except ForOfLoopControl to BCEHelper.{cpp.h}.

Part 2 moves NestableControl and subclasses to BCEHelper.{cpp.h},
except ForOfLoopControl which has dependency for TryEmitter.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Move EmitterScope to EmitterScope.{cpp.h}.

Part 3 moves EmitterScope to EmitterScope.{cpp.h}.
Because this has no dependency for remaining classes, I put it in its own files.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Move IfEmitter to BCEHelper.{cpp.h}.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Move TryEmitter and ForOfLoopControl to BCEHelper.{cpp.h}.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8987531 [details] [diff] [review]
Part 1: Move TDZCheckCache to BCEHelper.{cpp.h}.

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

::: js/src/frontend/BCEHelper.h
@@ +6,5 @@
> +
> +/* BytecodeEmitter's helper functions/classes. */
> +
> +#ifndef frontend_BCEHelper_h
> +#define frontend_BCEHelper_h

I'd just put TDZCheckCache in its own header.  BCEHelper is only a half-improvement IMO, and files multiplying is not a bad thing.

@@ +8,5 @@
> +
> +#ifndef frontend_BCEHelper_h
> +#define frontend_BCEHelper_h
> +
> +#include "frontend/BytecodeEmitter.h"

Please throw some IWYU-fu at this -- ds/Nestable.h, mozilla/Attributes.h, mozilla/Maybe.h, and any other headers to make this compile, but not the BE.h mega-header.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8987531 [details] [diff] [review]
Part 1: Move TDZCheckCache to BCEHelper.{cpp.h}.

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

::: js/src/frontend/BCEHelper.h
@@ +29,5 @@
> +// because a prior check would have already failed.  Finally, because
> +// evaluating a lexical variable declaration initializes it (after any
> +// initializer is evaluated), evaluating a lexical declaration marks its entry
> +// as DontCheckTDZ.
> +class BytecodeEmitter::TDZCheckCache : public Nestable<BytecodeEmitter::TDZCheckCache>

Actually, I guess as long as it's a nested class you have to depend on BCE.h.  Could that be changed to make it not one, maybe make the class member type be a |using|?

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8987532 [details] [diff] [review]
Part 2: Move NestableControl classes except ForOfLoopControl to BCEHelper.{cpp.h}.

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

::: js/src/frontend/BCEHelper.h
@@ +47,5 @@
>      mozilla::Maybe<MaybeCheckTDZ> needsTDZCheck(BytecodeEmitter* bce, JSAtom* name);
>      MOZ_MUST_USE bool noteTDZCheck(BytecodeEmitter* bce, JSAtom* name, MaybeCheckTDZ check);
>  };
>  
> +class BytecodeEmitter::NestableControl : public Nestable<BytecodeEmitter::NestableControl>

BytecodeControlStructures.h for this header?  And similar name for the file?

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8987533 [details] [diff] [review]
Part 3: Move EmitterScope to EmitterScope.{cpp.h}.

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

(I should say I'm assuming all these moves are just copypasta without extra change.)

::: js/src/frontend/EmitterScope.h
@@ +6,5 @@
> +
> +#ifndef frontend_EmitterScope_h
> +#define frontend_EmitterScope_h
> +
> +#include "frontend/BytecodeEmitter.h"

Could this be EmitterScope.h/cpp?  Plus make this #include list not depend on BE.h and be comprehensive.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8987534 [details] [diff] [review]
Part 4: Move IfEmitter to BCEHelper.{cpp.h}.

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

::: js/src/frontend/BCEHelper.h
@@ +47,5 @@
>      mozilla::Maybe<MaybeCheckTDZ> needsTDZCheck(BytecodeEmitter* bce, JSAtom* name);
>      MOZ_MUST_USE bool noteTDZCheck(BytecodeEmitter* bce, JSAtom* name, MaybeCheckTDZ check);
>  };
>  
> +// Class for emitting bytecode for blocks like if-then-else.

Could this all be in IfEmitter.cpp/h?

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8987535 [details] [diff] [review]
Part 5: Move TryEmitter and ForOfLoopControl to BCEHelper.{cpp.h}.

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

::: js/src/frontend/BCEHelper.h
@@ +414,5 @@
> +//     tryCatch.emitFinally(Some(finally_pos));
> +//     emit(finally_block);
> +//     tryCatch.emitEnd();
> +//
> +class MOZ_STACK_CLASS TryEmitter

Can we have TryEmitter.h/cpp files and ForOfControl.h/cpp files?

Comment 12

[Jeff Walden [:Waldo]]

Obviously these r+s are a little bit aggressive; use your judgment as to whether moving things into a wider variety of headers requires fresh reviews and all.  Just, having only one more header/file seems like not quite enough improvement to me, IMO.

Comment 13

[Tooru Fujisawa [:arai]]

Thank you for reviewing!

(In reply to Jeff Walden [:Waldo] from comment #9)
> Could this be EmitterScope.h/cpp?  Plus make this #include list not depend
> on BE.h and be comprehensive.

the following "friend" declaration is problematic if we remove BE.h dependency:

class EmitterScope : public Nestable<EmitterScope>
{
...
  friend bool BytecodeEmitter::needsImplicitThis();
};

I'll look for some workaround.

Comment 14

[Tooru Fujisawa [:arai]]

err... apparently the friend declaration isn't necessary at all.
I'll just remove it.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Include necessary headers in Nestable.h.

just added missing includes.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move TDZCheckCache to TDZCheckCache.{cpp.h}.

there's only trivial change. carrying forward r+

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Move EmitterScope to EmitterScope.{cpp.h}.

only trivial change.
reordered because of dependency.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Move NestableControl classes except ForOfLoopControl to BytecodeControlStructures.{cpp.h}.

Moved classes out of BytecodeEmitter class, and also separated JumpList/JumpTarget into their own file, because of dependency.

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Move IfEmitter to IfEmitter.{cpp.h}.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Move TryEmitter and ForOfLoopControl to TryEmitter.{cpp.h} and ForOfLoopControl.{cpp.h} .

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 8991196 [details] [diff] [review]
Part 3: Move NestableControl classes except ForOfLoopControl to BytecodeControlStructures.{cpp.h}.

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

::: js/src/frontend/BytecodeControlStructures.cpp
@@ +13,5 @@
> +using namespace js::frontend;
> +
> +NestableControl::NestableControl(BytecodeEmitter* bce, StatementKind kind)
> +  : Nestable<NestableControl>(&bce->innermostNestableControl)
> +  , kind_(kind)

While you're moving this over, maybe put the commas at end of line in these cases?  I see a couple others in this file that should be made similarly consistent with our style too.

::: js/src/frontend/JumpList.h
@@ +56,5 @@
> +
> +struct JumpList {
> +    // -1 is used to mark the end of jump lists.
> +    JumpList() : offset(-1) {}
> +    ptrdiff_t offset;

Use an = -1 member-initializer for this.

@@ +59,5 @@
> +    JumpList() : offset(-1) {}
> +    ptrdiff_t offset;
> +
> +    // Add a jump instruction to the list.
> +    void push(jsbytecode* code, ptrdiff_t jumpOffset);

Add #include <stddef.h> for ptrdiff_t.

Comment 22

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88dade249df039296591aa949ecbd122b2f20deb
Bug 1460489 - Part 0: Include necessary headers in Nestable.h. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/9392aa3091188fafba39e6fd4d3781b13eac6341
Bug 1460489 - Part 1: Move TDZCheckCache to TDZCheckCache.{cpp.h}. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/73895cf7ece580c7acb74830d2afae111e652110
Bug 1460489 - Part 2: Move EmitterScope to EmitterScope.{cpp.h}. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bdd1b1c3fb8326357febb481f9015fccc73b06c
Bug 1460489 - Part 3: Move NestableControl classes except ForOfLoopControl to BytecodeControlStructures.{cpp.h}. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f631b52845bdd5a67e9d284f38d69a58bd47e36
Bug 1460489 - Part 4: Move IfEmitter to IfEmitter.{cpp.h}. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aeed50db2a79de718e0f9688d48531f9ddc4630
Bug 1460489 - Part 5: Move TryEmitter and ForOfLoopControl to TryEmitter.{cpp.h} and ForOfLoopControl.{cpp.h} . r=jwalden

Comment 23

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a29c6057ad4c67c1f840cad5acbb66e7249a18e
Bug 1460489 - followup: Remove inline from BackgroundSweepTask::{isRunning,isRunningWithLockHeld} definitions which is called from other files. r=bustage CLOSED TREE

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/88dade249df0
https://hg.mozilla.org/mozilla-central/rev/9392aa309118
https://hg.mozilla.org/mozilla-central/rev/73895cf7ece5
https://hg.mozilla.org/mozilla-central/rev/2bdd1b1c3fb8
https://hg.mozilla.org/mozilla-central/rev/1f631b52845b
https://hg.mozilla.org/mozilla-central/rev/8aeed50db2a7
https://hg.mozilla.org/mozilla-central/rev/6a29c6057ad4