Closed
Bug 1260871
Opened 8 years ago
Closed 8 years ago
Remove the do_GetAtom() functions
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file, 1 obsolete file)
165.19 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
We have four NS_NewAtom() functions, and also four do_GetAtom() functions that are just synonyms. There's no point having both.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8736450 -
Flags: review?(erahm)
Attachment #8736450 -
Flags: feedback?(nfroyd)
Comment 2•8 years ago
|
||
Comment on attachment 8736450 [details] [diff] [review] Remove the do_GetAtom() functions Review of attachment 8736450 [details] [diff] [review]: ----------------------------------------------------------------- I have a slight preference for the do_GetAtom form, as the NS_NewAtom suggests allocating a distinct object, which is not necessarily the case. do_GetAtom (or, better, do_InternAtom; lispftw!) communicates the effect slightly better. But I'm happy with whatever color Eric wants to paint the bikeshed.
Attachment #8736450 -
Flags: feedback?(nfroyd) → feedback+
Assignee | ||
Comment 3•8 years ago
|
||
> I have a slight preference for the do_GetAtom form, as the NS_NewAtom
> suggests allocating a distinct object, which is not necessarily the case.
> do_GetAtom (or, better, do_InternAtom; lispftw!) communicates the effect
> slightly better.
>
> But I'm happy with whatever color Eric wants to paint the bikeshed.
I actually want to bikeshed this a bit...
- do_GetAtom() is the more popular form, with 230 uses vs. 40 for NS_NewAtom().
- But I don't much like the |do_| prefix.
- But the "New" in NS_NewAtom() doesn't communicate very well the notion that it'll return an existing atom if one exists.
- And we also have NS_GetStaticAtom(), which looks for an existing static atom but returns |nullptr| on failure. So that use of "Get" is different to the use in do_GetAtom(), which is infallible. (Having said that, NS_GetStaticAtom() also has only a handful of users and I wonder if it could be removed entirely.)
With all that in mind, I'd be happy with an entirely new name. I suggest NS_Atomize(), which would match SpiderMonkey's js::Atomize().
Comment 4•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #3) > > I have a slight preference for the do_GetAtom form, as the NS_NewAtom > > suggests allocating a distinct object, which is not necessarily the case. > > do_GetAtom (or, better, do_InternAtom; lispftw!) communicates the effect > > slightly better. > > > > But I'm happy with whatever color Eric wants to paint the bikeshed. > > I actually want to bikeshed this a bit... > > - do_GetAtom() is the more popular form, with 230 uses vs. 40 for > NS_NewAtom(). > > - But I don't much like the |do_| prefix. > > - But the "New" in NS_NewAtom() doesn't communicate very well the notion > that it'll return an existing atom if one exists. > > - And we also have NS_GetStaticAtom(), which looks for an existing static > atom but returns |nullptr| on failure. So that use of "Get" is different to > the use in do_GetAtom(), which is infallible. (Having said that, > NS_GetStaticAtom() also has only a handful of users and I wonder if it could > be removed entirely.) > > With all that in mind, I'd be happy with an entirely new name. I suggest > NS_Atomize(), which would match SpiderMonkey's js::Atomize(). I agree going with the more prevalent one makes sense and the "New" prefixed one is a bit misleading. The "do_" prefix is a bit odd, but it's pretty standard across our codebase. Just switching NS_NewAtom => do_GetAtom would certainly be easier to review ;) If you feel super strongly about it I'm good with NS_Atomize, splitting it into two patches would be helpful (rename NS_NewAtom instances to do_GetAtom, rename everything to NS_Atomize).
Comment 5•8 years ago
|
||
NS_Atomize sounds good to me. I think ditching the do_ prefix is a good thing, as we tend to use it only for things that would be significantly more complicated if you weren't using do_*.
Comment 6•8 years ago
|
||
Comment on attachment 8736450 [details] [diff] [review] Remove the do_GetAtom() functions Clearing for now, it looks like we want to go with wholesale conversion to |NS_Atomize|.
Attachment #8736450 -
Flags: review?(erahm)
Assignee | ||
Comment 7•8 years ago
|
||
New version using NS_Atomize(). Sorry, I didn't see your suggestion to split the patch in two until after I'd done it, at which point I didn't want to redo it... but I think it's not too hard to read in this form.
Attachment #8736524 -
Flags: review?(erahm)
Assignee | ||
Updated•8 years ago
|
Attachment #8736450 -
Attachment is obsolete: true
Comment 8•8 years ago
|
||
Comment on attachment 8736524 [details] [diff] [review] Remove do_GetAtom() and rename NS_NewAtom() as NS_Atomize() Review of attachment 8736524 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #8736524 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 9•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9146977af0d77727c5f81a75943b1c4dd7a2f38d Bug 1260871 - Remove do_GetAtom() and rename NS_NewAtom() as NS_Atomize(). r=erahm.
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9146977af0d7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•