Closed
Bug 1186724
Opened 9 years ago
Closed 9 years ago
Make BytecodeEmitter::emit2 take a uint8_t instead of a jsbtyecode
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: efaust, Assigned: mrrrgn, Mentored)
Details
(Whiteboard: [good first bug][lang=c++])
Attachments
(1 file, 1 obsolete file)
16.82 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
As near as I can tell, we never have jsbytecode running around, and we almost certainly don't have it laying around for the second operand value. Instead, we should be clear that we expect an arbitrary byte there. This will also alleviate the needs for tons of spurious casting all over the emitter.
Comment 1•9 years ago
|
||
This should be a good first bug :) Required code changes are following: * Change the type of the second argument (op1) of BytecodeEmitter::emit2 [1] to uint8_t * Rewrite all BytecodeEmitter::emit2 callsites to follow the change (remove cast to jsbytecode, or add cast to uint8_t) You'll need basic knowledge of C++, and be able to build and test Firefox [2] or SpiderMonkey [3]. A skilled first-time contributor should be able to finish this in a month. Leave comments / questions here, or ask me (:arai) in IRC #introduction and #jsapi channels. [1] https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/js/src/frontend/BytecodeEmitter.cpp#l247 (note that I linked to specific revision of the file to point the method, it may not be the latest one) [2] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build [3] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
Mentor: arai.unmht
Whiteboard: [good first bug][lang=c++]
Assignee | ||
Comment 3•9 years ago
|
||
Try Push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e594db110318
Attachment #8645393 -
Flags: review?(arai.unmht)
Comment 4•9 years ago
|
||
Comment on attachment 8645393 [details] [diff] [review] spidermonkey-jsbytecode-to-uint8_t.diff Review of attachment 8645393 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for your patch :D It's almost perfect, with following minor fixes. https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/js/src/frontend/BytecodeEmitter.cpp#4348 > jsbytecode offset = 1; https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/js/src/frontend/BytecodeEmitter.cpp#4509 > if (offset != 1 && !emit2(JSOP_PICK, offset - 1)) I think there's no reason to keep the type of `offset` as `jsbytecode` now, please make it `uint8_t` as well. ::: js/src/frontend/BytecodeEmitter.cpp @@ +260,2 @@ > code[0] = jsbytecode(op); > code[1] = op1; Type of `code` should remain `jsbytecode*`, as other methods do so and next line does cast. Instead, it would be better to add cast to `code[1] = op1;` line too. @@ +2921,5 @@ > + if (!emit2(JSOP_PICK, 4)) // OBJ N N+1 KEY THIS > + return false; > + if (!emit2(JSOP_PICK, 4)) // N N+1 KEY THIS OBJ > + return false; > + if (!emit2(JSOP_PICK, 3)) // N KEY THIS OBJ N+1 Please replace HTABs in above lines with SPACEs ;)
Attachment #8645393 -
Flags: review?(arai.unmht) → feedback+
Assignee | ||
Comment 5•9 years ago
|
||
Got it, thank you for the review! :)
Attachment #8645393 -
Attachment is obsolete: true
Attachment #8645401 -
Flags: review?(arai.unmht)
Comment 6•9 years ago
|
||
Comment on attachment 8645401 [details] [diff] [review] spidermonkey-jsbytecode-to-uint8_t.diff Review of attachment 8645401 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +2943,5 @@ > return emit1(JSOP_ZERO); > if (ival == 1) > return emit1(JSOP_ONE); > if ((int)(int8_t)ival == ival) > + return emit2(JSOP_INT8, (int8_t)ival); This doesn't cause warnings about signedness in GCC or something? `uint8_t(int8_t(ival))` would be ok. In general we prefer C++-style function-like casts to C-style casts.
Comment 7•9 years ago
|
||
Comment on attachment 8645401 [details] [diff] [review] spidermonkey-jsbytecode-to-uint8_t.diff Review of attachment 8645401 [details] [diff] [review]: ----------------------------------------------------------------- Thank you again :) r+ with those cast fixes that jorendorff pointed out.
Attachment #8645401 -
Flags: review?(arai.unmht) → review+
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a2be047f4e05
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•