Closed Bug 792169 Opened 8 years ago Closed 8 years ago

add NS_NewIMutableArray

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file, 1 obsolete file)

In the interest of dispensing with nsISupportsArray, it would be nice if nsIMutableArray had a corresponding constructor function to NS_NewISupportsArray.

Granted, there's nsArrayConstructor, but that's internal to nsArray and the xpcom module, and requires mucking about with IIDs and whatnot.
Attached patch patch (obsolete) — Splinter Review
Attachment #662270 - Flags: review?(benjamin)
Why not just "new nsArray"?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Why not just "new nsArray"?

nsArray.h is not exported.

nsArrayConstructor also contains a bit of logic to create cycle-collectable arrays if we're running on the main thread.  It seemed unwise to require people to know which kind of array they wanted.
Hrm. I'd really like this to be a C++-y as possible, not perpetuating the old and generally bad NS_New structures.

Why not do this:

* make the nsArray constructor protected
* add a `public: static already_addRefed<nsIArray> Create()` and a comment about how it will create a cycle-collectable array on the main thread and a non-CC array on other threads
Comment on attachment 662270 [details] [diff] [review]
patch

OK, I can try to do something like that.  I thought the whole point of the NS_New* functions was to avoid exporting the internals of the implementation and/or providing more efficient/direct ways to create things without having to go through do_CreateInstance.  I guess that's not the case?
Attachment #662270 - Flags: review?(benjamin)
Avoiding do_CreateInstance, yes (although unless it's in tight loops it doesn't show up in profiles, it's just ugly). Avoiding exporting may have been a goal in the past, but I don't think it's really worthwhile. Let's embrace our C++-ness.
Attached patch revised patchSplinter Review
In the interest of safety, I made the constructors non-public and moved the XPCOM constructor function into nsArray itself.  I also cleaned up the comment block before nsArray; it doesn't look like any of it is applicable anymore.

The one thing I'm not sure about is losing the null check in nsArray::Create as opposed to nsArray::XPCOMConstructor.  But ISTR that new is infallible, or has become so lately...?
Attachment #662270 - Attachment is obsolete: true
Attachment #663430 - Flags: review?(benjamin)
Comment on attachment 663430 [details] [diff] [review]
revised patch

Yes, new is infallible now so you can safely remove the null-check.
Attachment #663430 - Flags: review?(benjamin) → review+
Given comment 8, I took the liberty of removing the null check from XPCOMConstructor, too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/711db2ce69f0
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/711db2ce69f0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.