Closed
Bug 1440610
Opened 6 years ago
Closed 6 years ago
ArenaLists::placeholder could have a better name
Categories
(Core :: JavaScript: GC, enhancement, P3)
Core
JavaScript: GC
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)
1.84 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
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
Updated•6 years ago
|
Priority: -- → P3
Reporter | ||
Updated•6 years ago
|
Mentor: jcoppeard
Updated•6 years ago
|
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 2•6 years ago
|
||
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•6 years ago
|
||
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?
Comment 5•6 years ago
|
||
(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
Comment 6•6 years ago
|
||
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.
Comment 7•6 years ago
|
||
(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
Updated•6 years ago
|
Assignee: nobody → bobslept
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e2debb29096
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•