Closed Bug 78439 Opened 23 years ago Closed 23 years ago

Implement do_GetAtom

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sfraser_bugs, Assigned: scc)

Details

Attachments

(2 files)

Spun off from bug 74948, we should have a do_GetAtom, so that you can write code 
like:

  nsCOMPtr<nsIAtom> fooAtom = do_GetAtom("foo");
and
  nsCOMPtr<nsIAtom> fooAtom = do_GetAtom("foo", &rv);
I tested (for one occurrence) that the following two statements (not exactly
these two, but anyway) lead to the same binary code in an optimized build:

nsCOMPtr<nsIAtom> myAtom = dont_AddRef(NS_NewAtom("foo"));
nsCOMPtr<nsIAtom> myAtom = do_NewAtom("foo");

I prefer do_NewAtom to do_GetAtom, but I could be convinced otherwise.
I prefer Get, since NS_NewAtom often just hands back an addreffed nsIAtom that it 
found in the global atoms hash table.
FWIW, there's no point taking an nsresult* as an optional second parameter
since NS_NewAtom doesn't return an nsresult.  It returns null as a sign of
failure.  The nsCOMPtr machinery that this uses should all be perfectly
happy with null, so the caller can still null-check the result if desired.

(Unless you want to change NS_NewAtom as well, that is...)
I like Get better.  New does not equivocate enough to suggest that it often
returns an old atom rather than creating a new one.

/be
I like the `Get' as well.  I think, though, that I would fix the various
versions of |NS_NewAtom| to return an |already_AddRefed<nsIAtom>| as per bug
#59212, then providing |do_GetAtom| as a traditional |nsCOMPtr_helper|, as per
other helpers (if we still want |do_GetAtom| at that point).  Although this sort
of falls between the cases since the underlying routine, |NS_NewAtom| isn't the
paremeter-result form returning status that all current |do_...| routines fix,
it would be a surprise to readers that this |do_...| was different from all the
others, and that this |do_...| would leak when not assigned.
The changes in bug 59212 aren't going to be practical after bug 75504 is fixed.
It would be nice to have an equivalent to those changes that is as efficient as
they are, although I guess the extra cost of a virtual function call isn't that
serious.

Will people really expect do_GetAtom() not to leak if they don't assign it to
anything?  (Does anybody currently rely on that behavior of do_GetService and
do_CreateInstance?)
How about making 
nsresult NS_GetAtom(..., nsIAtom** aAtom)
and using it in the do_GetAtom?
Changing platform
Hardware: All → Macintosh
OS: Mac System 8.5 → All
Hardware: Macintosh → All
This went in as part of bug 92141.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: