The default bug view has changed. See this FAQ.

IonMonkey: JSOP*PROP and JSOP*NAME ops should use PropertyName instead of JSAtom

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jkitch)

Tracking

(Blocks: 1 bug)

unspecified
mozilla18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++][mentor=jandem][ion:t])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Filing this bug for the IonMonkey part of Waldo's patch in bug 713183.

(This may also be a good first bug.)
Whiteboard: [lang=c++][mentor=jandem]
Whiteboard: [lang=c++][mentor=jandem] → [lang=c++][mentor=jandem][ion:t]
(Assignee)

Comment 1

5 years ago
(In reply to Jan de Mooij [:jandem] from comment #0)
> Filing this bug for the IonMonkey part of Waldo's patch in bug 713183.
> 
> (This may also be a good first bug.)

I would like to work on this as my first bug.
(Reporter)

Comment 2

5 years ago
(In reply to James Kitchener from comment #1)
> 
> I would like to work on this as my first bug.

Awesome! I assigned this bug to you.

Sean pointed out on IRC that only JSOP_DELPROP (MDeleteProperty) has not yet been updated. If you compare MDeleteProperty and MCallGetProperty you will notice that the former has a JSAtom * member and the latter has a CompilerRootPropertyName/HandlePropertyName. We should also rename the "atom()" method to "name()".

You will also have to update IonBuilder::jsop_delprop to take a HandlePropertyName, just like IonBuilder::jsop_getprop.

See this page for information on how to build the SpiderMonkey shell:
https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey

Let me know if you have any questions, or ask in #jsapi on IRC.
Assignee: general → jkitch.bug
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 661482 [details] [diff] [review]
Replace JSAtom with PropertyName in JSOP_DELPROP

The changes have been made.

It passes the tests mentioned in the New to Spidermonkey page.  Are there any others I should run?

(Well it passes except for 19 ecma\Date tests and js1_5\extensions\toLocaleFormat-01.js which also failed without my modifications)
(Assignee)

Comment 4

5 years ago
Created attachment 661572 [details] [diff] [review]
Replace JSAtom with PropertyName in JSOP_DELPROP

fix whitespace and flagging for review
Attachment #661482 - Attachment is obsolete: true
Attachment #661572 - Flags: review?(jdemooij)
(Reporter)

Comment 5

5 years ago
Comment on attachment 661572 [details] [diff] [review]
Replace JSAtom with PropertyName in JSOP_DELPROP

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

Excellent, thanks! I will land the patch right now.

These test failures are expected, they fail when you are in a time zone other than PDT.

For IonMonkey development it's good to run jit-tests with the --ion flag:

jit-test/jit_test.py --ion $build/js

This runs every test twice: once with --ion-eager and once with --no-jm. By default, IonMonkey compiles only really hot functions (> ~10000 iterations/calls) and these flags lower that threshold to 0 and 40 respectively. This greatly improves test coverage. We should update the "New to SpiderMonkey" page and maybe change the jit-test harness to run tests with --ion-eager by default, I will file a new bug for that.
Attachment #661572 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 6

5 years ago
Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d18e2101417

Thanks again!
https://hg.mozilla.org/mozilla-central/rev/2d18e2101417
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.