Closed
Bug 1199710
Opened 9 years ago
Closed 7 years ago
Use "enum class" for ExitFrame tokens.
Categories
(Core :: JavaScript Engine: JIT, defect, P5)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: nbp, Assigned: keremkat, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
23.27 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Assignee: nobody → evilpies
Comment 1•8 years ago
|
||
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)
Reporter | ||
Comment 3•7 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+
(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)
Reporter | ||
Comment 6•7 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+
Reporter | ||
Comment 8•7 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
Attachment #8904653 -
Attachment is obsolete: true
Attachment #8905668 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 10•7 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•7 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•7 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•7 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+
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b550eb6afd78
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Assignee: nobody → keremkat
You need to log in
before you can comment on or make changes to this bug.
Description
•