Closed Bug 1199710 Opened 9 years ago Closed 7 years ago

Use "enum class" for ExitFrame tokens.

Categories

(Core :: JavaScript Engine: JIT, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox43 --- affected
firefox57 --- fixed

People

(Reporter: nbp, Assigned: keremkat, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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
Attachment #8904355 - Flags: review?(nicolas.b.pierron)
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+
(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.
Attachment #8904355 - Attachment is obsolete: true
Attachment #8904653 - Flags: review?(nicolas.b.pierron)
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+
I do not have commit nor try access, thanks.
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
Attachment #8904653 - Attachment is obsolete: true
Attachment #8905668 - Flags: review?(nicolas.b.pierron)
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+
Fixed the nit and included Trampoline-x64.cpp changes.
Attachment #8905668 - Attachment is obsolete: true
Attachment #8906754 - Flags: review?(nicolas.b.pierron)
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b550eb6afd78
ExitFrameTokenValues enum is refactored into enum class ExitFrameToken. r=nbp
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
Closed: 7 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.