ArenaLists::placeholder could have a better name

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: jonco, Assigned: bobslept, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla62
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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

a year ago
Mentor: jcoppeard

Updated

11 months ago
Whiteboard: [lang=c++]
(Assignee)

Comment 1

10 months ago
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

10 months 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

10 months 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+
(Assignee)

Comment 4

10 months ago
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

10 months 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
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

10 months 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!

Comment 8

10 months ago
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e2debb29096
Rename ArenaLists::placeholder to ArenaLists::emptySentinel. r=jonco

Updated

10 months ago
Assignee: nobody → bobslept
https://hg.mozilla.org/mozilla-central/rev/8e2debb29096
Status: NEW → RESOLVED
Last Resolved: 10 months 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.