ArenaLists::placeholder could have a better name

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: jonco, Assigned: bobslept, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment)

Reporter

Description

Last year
This is a sentinel object used to indicate an empty free span.  So it should probably be called 'emptySentinel' or something like that.

https://searchfox.org/mozilla-central/search?q=symbol:_ZN2js2gc10ArenaLists11placeholderE&redirect=false
Priority: -- → P3
Reporter

Updated

Last year
Mentor: jcoppeard
Whiteboard: [lang=c++]
Assignee

Comment 1

Last year
I hope this patch is correct. I have renamed placeholder to emptySentinel. It builds and runs alright. Do I need to run a specific test suite to be totally sure?
Comment on attachment 8975282 [details] [diff] [review]
1440610-rename-placeholder.patch

Thanks for the patch! Probably running the jsapi-tests would be good enough, |./mach jsapi-tests|. Lets go ahead and ask Jon for a review.
Attachment #8975282 - Flags: review?(jcoppeard)
Reporter

Comment 3

Last year
Comment on attachment 8975282 [details] [diff] [review]
1440610-rename-placeholder.patch

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

Looks good to me!
Attachment #8975282 - Flags: review?(jcoppeard) → review+
Assignee

Comment 4

Last year
Well that is great news! If I'm right, I now need to ask you to push it to the try server for testing?
After successful testing, is it me or the bug owner that needs to add the checkin-needed keywords?
(In reply to bobslept from comment #4)
> Well that is great news! If I'm right, I now need to ask you to push it to
> the try server for testing?
> After successful testing, is it me or the bug owner that needs to add the
> checkin-needed keywords?

Once we have a good try build you can add 'checkin-needed'. Before that can you update the patch title to include "r=jonco" at the end? Go ahead adn mark the updated patch r+ as Jon already approved it.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5f390d6e15af92c2eb1f781c552197712fbd26c
bobslept might not have the editbug privileges needed to do either of those things. Just let us know and we can do it for you.
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #6)
> bobslept might not have the editbug privileges needed to do either of those
> things. Just let us know and we can do it for you.

Ah good point, try looks good. I'll get this landed. Thank you for your contribution bobslept!

Comment 8

Last year
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e2debb29096
Rename ArenaLists::placeholder to ArenaLists::emptySentinel. r=jonco
Assignee: nobody → bobslept
https://hg.mozilla.org/mozilla-central/rev/8e2debb29096
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.