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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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

(This may also be a good first bug.)

Comment 1

[James Kitchener (:jkitch)]

(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.

Comment 2

[Jan de Mooij [:jandem]]

(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.

Comment 3

[James Kitchener (:jkitch)]

Attachment: 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)

Comment 4

[James Kitchener (:jkitch)]

Attachment: Replace JSAtom with PropertyName in JSOP_DELPROP

fix whitespace and flagging for review

Comment 5

[Jan de Mooij [:jandem]]

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.

Comment 6

[Jan de Mooij [:jandem]]

Landed:

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

Thanks again!

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2d18e2101417