Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: jandem, Assigned: spiro)

Tracking

({triage-deferred})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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().
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.
Having it hold a JS::Value is interesting but unfortunately it will make things like js::Shape 8 bytes bigger on 32-bit.
Keywords: triage-deferred
Priority: -- → P3
Renamed jsid to PropertyKey and included in JS namespace
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/65510a1c9b0f
Renamed jsid to PropertyKey and included in JS namespace. r=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.
Assignee: nobody → sharma.divyansh.501
Status: NEW → ASSIGNED
Flags: needinfo?(sharma.divyansh.501)
Keywords: leave-open
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.
Jan

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

Thanks.
Flags: needinfo?(sharma.divyansh.501)
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?
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
set JSID_IS_INT to PropertyKey::isInt() as first patch. 
Do not close this diff after acceptance!
set JSID_IS_INT to PropertyKey::isInt()
Attachment #9030079 - Attachment description: Bug 1320105 : set JSID_IS_INT to PropertyKey::isInt() → Bug 1320105 : Changes JSID_IS_INT to PropertyKey::isInt()
Attachment #9030079 - Attachment description: Bug 1320105 : Changes JSID_IS_INT to PropertyKey::isInt() → Bug 1320105 : Convert JSID_IS_INT to PropertyKey::isInt()
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/968418409443
Convert JSID_IS_INT to PropertyKey::isInt() r=jandem
Keywords: checkin-needed
Keywords: checkin-needed
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.
Flags: needinfo?(sharma.divyansh.501)
Keywords: checkin-needed
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?
Flags: needinfo?(sharma.divyansh.501) → needinfo?(jdemooij)
(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.
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Flags: needinfo?(jdemooij)
Resolution: --- → FIXED
Jan

So no need for further patches?
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.