1320105 - Drag jsid into the 21st century

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jan de Mooij [:jandem]]

It would be nice to refactor jsid to be more like JS::Value. We could rename it to JS::PropertyKey (I think that's what the spec uses?) and replace JSID_TO_INT etc with methods like isInt().

Comment 1

[Jeff Walden [:Waldo]]

Rename it, make it directly hold a JS::Value, yeah.  It's kinda nuts we have two different encodings for this, versus just having one and considerably simplifying life.

I remember starting to do some of the signature-work to make this possible, a long time ago, but I never followed up on it quickly enough for it to not get ripped out.

Comment 2

[Jan de Mooij [:jandem]]

Having it hold a JS::Value is interesting but unfortunately it will make things like js::Shape 8 bytes bigger on 32-bit.

Comment 3

[Divyansh Sharma [:spiro]]

Attachment: Bug 1320105 : Renamed jsid to PropertyKey and included in JS namespace.

Renamed jsid to PropertyKey and included in JS namespace

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65510a1c9b0f
Renamed jsid to PropertyKey and included in JS namespace. r=jandem

Comment 5

[Jan de Mooij [:jandem]]

Great start. Are you interested in making the follow-up changes too?

There are many things we can do now, for example:

* Start using |PropertyKey key| instead of |jsid id|.
* Rename RootedId/HandleId etc to RootedPropertyKey.
* Change functions like JSID_IS_INT to methods on PropertyKey so we can do key.isInt().

I'll mark this as leave-open for now but we could also do this in separate/follow-up bugs if you prefer.

Comment 6

[Jan de Mooij [:jandem]]

Oh and I'd suggest doing this incrementally where it's reasonable, for instance one directory at a time. Makes it easier to review and track down issues later.

Comment 7

[Divyansh Sharma [:spiro]]

Jan

I will be more than happy to contribute to further steps of this bug :)

Thanks.

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/65510a1c9b0f

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/65510a1c9b0f

Comment 10

[Divyansh Sharma [:spiro]]

Jan

I have renamed the methods in Id.h . Now if I make changes in directories one at a time, not all of the patches will be buildable. Is that fine by your side?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Divyansh Sharma [:spiro] from comment #10)
> I have renamed the methods in Id.h . Now if I make changes in directories
> one at a time, not all of the patches will be buildable. Is that fine by
> your side?

It's best to make sure each revision is buildable. One reason is that it helps people bisect in the future when there's a problem :) I'd suggest writing patches like this:

* First patch: Take one function, say JSID_IS_INT, and add it as PropertyKey::isInt. Change JSID_IS_INT to call id.isInt().

* More patches: Incrementally convert JSID_IS_INT(id) calls to id.isInt().

* Final patch: Remove JSID_IS_INT because it's now unused.

I would also file a new bug (blocking this one) for each function we convert to keep it organized.

Comment 12

[Divyansh Sharma [:spiro]]

Attachment: Bug 1320105 : set JSID_IS_INT to PropertyKey::isInt()

set JSID_IS_INT to PropertyKey::isInt() as first patch. 
Do not close this diff after acceptance!

Comment 13

[Divyansh Sharma [:spiro]]

Attachment: Bug 1320105 : Convert JSID_IS_INT to PropertyKey::isInt()

set JSID_IS_INT to PropertyKey::isInt()

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/968418409443
Convert JSID_IS_INT to PropertyKey::isInt() r=jandem

Comment 15

[Cosmin Sabou [:CosminS]]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmp7sXphQ\npatching file js/public/Id.h\nHunk #1 FAILED at 52\nHunk #2 FAILED at 90\n2 out of 2 hunks FAILED -- saving rejects to file js/public/Id.h.rej\nabort: patch failed to apply', '') 

This is probably because a prior diff was already landed as you can see in c#14. Please take a look over this.

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/968418409443

Comment 17

[Divyansh Sharma [:spiro]]

Jan

My previous diff has landed already. But in phabricator, it raised some clang related errors and I resolved them with new diff. But this diff is failing to land due to previous one's success. So what do I do in this situation? Can we mark this as resolved?

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Divyansh Sharma [:spiro] from comment #17)
> Jan
> 
> My previous diff has landed already. But in phabricator, it raised some
> clang related errors and I resolved them with new diff. But this diff is
> failing to land due to previous one's success. So what do I do in this
> situation? Can we mark this as resolved?

Yes let's close this and file new bugs for follow-up work :) Sylvestre is landing clang-format fixes every week or so for now, so we don't have to worry too much about it.

Comment 19

[Divyansh Sharma [:spiro]]

Jan

So no need for further patches?

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Divyansh Sharma [:spiro] from comment #19)
> Jan
> 
> So no need for further patches?

Not to address the clang-format issues. However we still want to convert JSID_IS_INT users to id.isInt() right? Please file a new bug for that if you want to work on it.