Closed
Bug 78439
Opened 23 years ago
Closed 23 years ago
Implement do_GetAtom
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: sfraser_bugs, Assigned: scc)
Details
Attachments
(2 files)
1.17 KB,
patch
|
Details | Diff | Splinter Review | |
1.17 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 3•23 years ago
|
||
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...)
Comment 6•23 years ago
|
||
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
Assignee | ||
Comment 7•23 years ago
|
||
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?)
Reporter | ||
Comment 9•23 years ago
|
||
How about making nsresult NS_GetAtom(..., nsIAtom** aAtom) and using it in the do_GetAtom?
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.
Description
•