Closed Bug 1260871 Opened 8 years ago Closed 8 years ago

Remove the do_GetAtom() functions

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(1 file, 1 obsolete file)

We have four NS_NewAtom() functions, and also four do_GetAtom() functions that
are just synonyms. There's no point having both.
Attachment #8736450 - Flags: review?(erahm)
Attachment #8736450 - Flags: feedback?(nfroyd)
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+
> 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().
(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).
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 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)
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)
Attachment #8736450 - Attachment is obsolete: true
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9146977af0d77727c5f81a75943b1c4dd7a2f38d
Bug 1260871 - Remove do_GetAtom() and rename NS_NewAtom() as NS_Atomize(). r=erahm.
https://hg.mozilla.org/mozilla-central/rev/9146977af0d7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.