Closed Bug 1440610 Opened 3 years ago Closed 3 years ago

ArenaLists::placeholder could have a better name

Categories

(Core :: JavaScript: GC, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jonco, Assigned: bobslept, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++])

Attachments

(1 file)

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
Mentor: jcoppeard
Whiteboard: [lang=c++]
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)
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+
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!
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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.