88344 - Add sorting capabilities in nsVoidArray & nsStringArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[rbs]

I am going to attach a patch that allows sorting the mImpl->mArray in
nsVoidArray. Due to the lack of an in-place sorting ability, people are copying 
the array in their own buffer when they are about to sort it. The patch will 
remdy the situation. 

The methods added:

  void nsVoidArray::Sort(nsVoidArrayComparatorFunc aFunc, void* aData);

  void nsStringArray::Sort(void);
  void nsStringArray::SortIgnoreCase(void);

  void nsCStringArray::Sort(void);
  void nsCStringArray::SortIgnoreCase(void);

They all boil down to calling NS_QuickSort on mImpl->mArray. And the string 
variants further default to the existing comparators in nsAString.h.

Comment 1

[rbs]

Attachment: patch - add Sort() to nsVoidArray

Comment 2

[Daniel Bratell]

Do you know where people are sorting the array themselves by copying so that we 
can file bugs on those and fix it?

Comment 3

[rbs]

Here is an example:

http://lxr.mozilla.org/seamonkey/source/intl/uconv/src/nsCharsetMenu.cpp#1471

Also, perhaps some people may be staying away from nsVoidArray in certain areas 
where they are anticipating that they will need sorting capabilities.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I had a similar patch for just nsVoidArray in bug 74920.  I preferred the API I
had for nsVoidArray, because while it was slightly less efficient, it was less
confusing, since the caller got their pointer back rather than a pointer to it.

Comment 5

[rbs]

Attachment: avoid pointer-to-pointer as preferred by dbaron

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

r=dbaron, if you've tested it appropriately

Comment 7

[rbs]

Tested (on Win32). I will sprinkle some debugging assertions to null check 
pointers and catch bad callers when checking in.

Comment 8

[rbs]

... and also use NS_STATIC_CAST per the local custom over there.

Comment 9

[rbs]

Attachment: Final patch - ready to checkin

Comment 10

[rbs]

OK, added the static cast -- didn't add the asertions (not in the local custom, 
and will crash immediately anyway), and fixed an XXX comment that was on my way, 
and which cvs blame said was a remnant of the heat of the XPCDOM landing.

Need sr= to be done with this one. Thanks.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

One additional comment that I just thought of:  it might be good to have (for
lack of widespread support for |using|) an inline method on ns[C]StringArray
that prevents the nsVoidArray::Sort from being hidden, i.e.:

void Sort(nsVoidArrayComparatorFunc aFunc, void* aData)
  {
     nsVoidArray::Sort(aFunc, aData);
  }

Comment 12

[rbs]

Attachment: OK, exposed the base Sort() everywhere

Comment 13

[rbs]

Attachment: cleaner/leaner/elegant patch in the new world

Comment 14

[Chris Waterson]

Let's have scc sr= this one, as he's the one with his heart setting on making
xpcom/ds a cleaner, safer, drug-free place.

Comment 15

[rbs]

scc, seeking your sr= to land this little gem. Thanks.

Comment 16

[Scott Collins]

works for me, sr=scc

Comment 17

[rbs]

Thanks guys, patch landed, resolving as FIXED.