Use "enum class" for ExitFrame tokens.

RESOLVED FIXED in Firefox 57

Status

()

defect
P5
normal
RESOLVED FIXED
4 years ago
7 months ago

People

(Reporter: nbp, Assigned: keremkat, Mentored)

Tracking

(Blocks 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox57 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
As suggestted by jandem in Bug 1184965 comment 20, ExitFrameToken should use "enum class", instead of the current prefixes that we always have.

This should be an easy bug which aims at compiling SpiderMonkey and getting your first patch reviewed.
Assignee: nobody → evilpies
This enum is called ExitFrameTokenValues. I would suggested renaming it back ExitFrameToken and shortening all the enum values by at least removing the trailing Token from the name.
Assignee: evilpies → nobody
Mentor: nicolas.b.pierron
Priority: -- → P5
(Assignee)

Comment 2

2 years ago
Attachment #8904355 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 3

2 years ago
Comment on attachment 8904355 [details] [diff] [review]
0001-enum-class-ExitFrameToken.patch

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

::: js/src/jit/JitFrames.h
@@ +493,5 @@
>  class IonOOLPropertyOpExitFrameLayout;
>  class IonOOLProxyExitFrameLayout;
>  class IonDOMExitFrameLayout;
>  
> +enum class ExitFrameToken : int32_t

nit: any reason to not use uint8_t ?

@@ +498,2 @@
>  {
> +    CallNativeExitFrameLayout        = 0x0,

This patch sounds good, but as Tom mentioned before, now that we are using enum calss, the names are quite redundant.
    *ExitFrame*Token::CallNative*ExitFrame*Layout

I suggest to remove all the  ExitFrame  from the enumerated value names, as well as the  Layout  , as these are tokens and not a layout of frames.

    ExitFrameToken::CallNative
    ExitFrameToken::IonDOMMethod
    ExitFrameToken::Bare
Attachment #8904355 - Flags: review?(nicolas.b.pierron) → feedback+
(Assignee)

Comment 4

2 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Comment on attachment 8904355 [details] [diff] [review]
> 0001-enum-class-ExitFrameToken.patch
> 
> Review of attachment 8904355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/JitFrames.h
> @@ +493,5 @@
> >  class IonOOLPropertyOpExitFrameLayout;
> >  class IonOOLProxyExitFrameLayout;
> >  class IonDOMExitFrameLayout;
> >  
> > +enum class ExitFrameToken : int32_t
> 
> nit: any reason to not use uint8_t ?
js::jit::Imm32 has a ctor that takes (int32_t)ExitFrameToken, that is the only reason. Changing to uint8_t in the new patch.

> 
> @@ +498,2 @@
> >  {
> > +    CallNativeExitFrameLayout        = 0x0,
> 
> This patch sounds good, but as Tom mentioned before, now that we are using
> enum calss, the names are quite redundant.
>     *ExitFrame*Token::CallNative*ExitFrame*Layout
> 
> I suggest to remove all the  ExitFrame  from the enumerated value names, as
> well as the  Layout  , as these are tokens and not a layout of frames.
> 
>     ExitFrameToken::CallNative
>     ExitFrameToken::IonDOMMethod
>     ExitFrameToken::Bare
Done.
(Assignee)

Comment 5

2 years ago
Attachment #8904355 - Attachment is obsolete: true
Attachment #8904653 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 6

2 years ago
Comment on attachment 8904653 [details] [diff] [review]
0002-enum-class-ExitFrameToken.patch

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

Thanks for this contribution, it is really nice to have shorter names while using an enum class!
Do you have commit access, or you need someone else to push the patch for you?

::: js/src/jit/MacroAssembler-inl.h
@@ +305,3 @@
>  {
>      linkExitFrame(cxreg, scratch);
> +    Push(Imm32((int32_t)token));

nit: use C++ constructor when possible:  int32_t(token)
Attachment #8904653 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 7

2 years ago
I do not have commit nor try access, thanks.
(Reporter)

Comment 8

2 years ago
Comment on attachment 8904653 [details] [diff] [review]
0002-enum-class-ExitFrameToken.patch

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

Please fix the nits, and I will send it to try once it compiles successfully locally.

::: js/src/jit/mips64/Trampoline-mips64.cpp
@@ +271,5 @@
>          masm.storePtr(zero, Address(StackPointer, 0)); // fake return address
>  
>          // No GC things to mark, push a bare token.
>          masm.loadJSContext(scratch);
> +        masm.enterFakeExitFrame(scratch, scratch, ExitFrameToken::ExitFrameLayoutBare);

nit: ExitFrameToken::Bare
(Assignee)

Comment 9

2 years ago
Attachment #8904653 - Attachment is obsolete: true
Attachment #8905668 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 10

2 years ago
Comment on attachment 8905668 [details] [diff] [review]
0003-enum-class-ExitFrameToken.patch

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

Fix js/src/jit/x64/Trampoline-x64.cpp, or include the change, and fix the nit mentioned in comment 6.
Attachment #8905668 - Flags: review?(nicolas.b.pierron) → feedback+
(Assignee)

Comment 11

2 years ago
Fixed the nit and included Trampoline-x64.cpp changes.
Attachment #8905668 - Attachment is obsolete: true
Attachment #8906754 - Flags: review?(nicolas.b.pierron)

Comment 12

2 years ago
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b550eb6afd78
ExitFrameTokenValues enum is refactored into enum class ExitFrameToken. r=nbp
(Reporter)

Comment 13

2 years ago
Comment on attachment 8906754 [details] [diff] [review]
0004-enum-class-ExitFrameToken.patch

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

Sounds good to me.

Tested on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58e5c610b041e36e39ccac7f1ed8496215c510f1
( and landed before publishing the review :/ )
Attachment #8906754 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/b550eb6afd78
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee: nobody → keremkat
You need to log in before you can comment on or make changes to this bug.