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

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks 1 bug)

Trunk
mozilla63
Points:
---

Firefox Tracking Flags

(firefox62 wontfix, firefox63 fixed)

Details

Attachments

(6 attachments, 5 obsolete attachments)

960 bytes, patch
Waldo
: review+
Details | Diff | Splinter Review
11.86 KB, patch
arai
: review+
Details | Diff | Splinter Review
89.05 KB, patch
arai
: review+
Details | Diff | Splinter Review
27.45 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
25.74 KB, patch
arai
: review+
Details | Diff | Splinter Review
49.33 KB, patch
arai
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
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.
Priority: -- → P3
(Assignee)

Comment 1

10 months ago
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)
Attachment #8987531 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

10 months ago
Part 2 moves NestableControl and subclasses to BCEHelper.{cpp.h},
except ForOfLoopControl which has dependency for TryEmitter.
Attachment #8987532 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

10 months ago
Part 3 moves EmitterScope to EmitterScope.{cpp.h}.
Because this has no dependency for remaining classes, I put it in its own files.
Attachment #8987533 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

10 months ago
Attachment #8987534 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

10 months ago
Attachment #8987535 - Flags: review?(jwalden+bmo)

Comment 6

10 months ago
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.
Attachment #8987531 - Flags: review?(jwalden+bmo) → review+

Comment 7

10 months ago
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

10 months ago
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?
Attachment #8987532 - Flags: review?(jwalden+bmo) → review+

Comment 9

10 months ago
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.
Attachment #8987533 - Flags: review?(jwalden+bmo) → review+
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?
Attachment #8987534 - Flags: review?(jwalden+bmo) → review+
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?
Attachment #8987535 - Flags: review?(jwalden+bmo) → review+
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.
(Assignee)

Comment 13

10 months ago
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.
(Assignee)

Comment 14

10 months ago
err... apparently the friend declaration isn't necessary at all.
I'll just remove it.
(Assignee)

Comment 15

10 months ago
just added missing includes.
Attachment #8991179 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 16

10 months ago
there's only trivial change. carrying forward r+
Attachment #8991180 - Flags: review+
(Assignee)

Comment 17

10 months ago
only trivial change.
reordered because of dependency.
Attachment #8987531 - Attachment is obsolete: true
Attachment #8987533 - Attachment is obsolete: true
Attachment #8991195 - Flags: review+
(Assignee)

Comment 18

10 months ago
Moved classes out of BytecodeEmitter class, and also separated JumpList/JumpTarget into their own file, because of dependency.
Attachment #8987532 - Attachment is obsolete: true
Attachment #8991196 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 19

10 months ago
Attachment #8987534 - Attachment is obsolete: true
Attachment #8991197 - Flags: review+

Updated

10 months ago
Attachment #8991179 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8991196 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 22

10 months ago
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
(Assignee)

Comment 23

10 months ago
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
You need to log in before you can comment on or make changes to this bug.