Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Review NS_QuickSort() documentation and uses: Solaris, qsort, ...

RESOLVED FIXED in mozilla17

Status

()

Core
XPCOM
--
minor
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: sgautherie, Assigned: Alexander Enaldiev)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=bsmedberg][lang=c++] [meta], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
http://mxr.mozilla.org/mozilla/source/xpcom/glue/nsQuickSort.h
{
39 /* We need this because Solaris' version of qsort is broken and
40  * causes array bounds reads.
41  */
}

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsQuickSort.cpp
{
32 /* We need this because Solaris' version of qsort is broken and
33  * causes array bounds reads.
34  */
}

***

(In reply to Wan-Teh Chang from bug 72196 comment #3)
> Mozilla should consider removing NS_QuickSort and just use
> qsort in the standard C library.  The comments in the source
> file:
>     /* We need this because Solaris' version of qsort is broken and
>      * causes array bounds reads.
>      */
> turned out to be wrong -- we were calling qsort with an
> incorrect comparison function which only broke the Solaris
> implementation of qsort.

(In reply to Jonas Sicking (:sicking) from bug 72196 comment #5)
> qsort doesn't provide a transparent |void* data| parameter that gets passed
> on
> to the compare-function. At least transformiix uses this, possibly others
> too.

Iiuc,
1) The Solaris comment(s) should be removed there too.
   And possibly replaced by the actual use case(s).
2) Replace morkQuickSort() with NS_QuickSort(), or document why that can't be.
3) Should review all (qsort() and) QuickSort() calls, wrt which function they should use.

http://mxr.mozilla.org/comm-central/search?string=qsort&case=on
"Found 103 matching lines in 55 files"
http://mxr.mozilla.org/comm-central/search?string=QuickSort&case=on
"Found 92 matching lines in 39 files"

Updated

5 years ago
Assignee: nobody → alexandr.jenaldiev
(Assignee)

Comment 1

5 years ago
Created attachment 612985 [details] [diff] [review]
Fix patch

Shame, but I'm not sure, that all target files contains in #include directives for <cstdlib> lib, which contains qsort(...)
(Assignee)

Comment 2

5 years ago
Need patch review. (especially `coz this is my first resolved bug!)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Updated

5 years ago
Attachment #612985 - Flags: review?(benjamin)
(Reporter)

Comment 3

5 years ago
Comment on attachment 612985 [details] [diff] [review]
Fix patch

(In reply to Serge Gautherie (:sgautherie) from comment #0)
> 3) Should review all (qsort() and) QuickSort() calls, wrt which function
> they should use.

Iiuc, what this patch does is to replace NS_QuickSort(..., <null>) calls with qsort() calls.

***

(In reply to Paladine from comment #1)
> Shame, but I'm not sure, that all target files contains in #include
> directives for <cstdlib> lib, which contains qsort(...)

I'm not sure what you meant.
Does this patch compile?
(Assignee)

Comment 4

5 years ago
Created attachment 613123 [details] [diff] [review]
replacement for NS_QuickSort

renewed.
>1) The Solaris comment(s) should be removed there too.
>   And possibly replaced by the actual use case(s).
>2) Replace morkQuickSort() with NS_QuickSort(), or document why that can't be.

Didn't saw anything of it.

>3) Should review all (qsort() and) QuickSort() calls, wrt which function they >should use.
When I saw causeless usage of NS_QuickSort, i removed it. 

Is I understanding the task (for this bug) right? Just saw the tag [good first bug] :)
Attachment #612985 - Attachment is obsolete: true
Attachment #612985 - Flags: review?(benjamin)
Attachment #613123 - Flags: review?(benjamin)
(Assignee)

Comment 5

5 years ago
Btw for me main question is: NS_QuickSort() was developed to provide qsort() on solaris? I have different interface (additional void* context parameter). I don't think, it can be replaced that way to easily.
Comment on attachment 613123 [details] [diff] [review]
replacement for NS_QuickSort

Thank you.
Attachment #613123 - Flags: review?(benjamin) → review+
Comment on attachment 613123 [details] [diff] [review]
replacement for NS_QuickSort

Review of attachment 613123 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSProps.cpp
@@ +220,5 @@
>  
>    // Sort with lowest count at the start and highest at the end, and
>    // within counts sort in reverse property index order.
> +  qsort(&subpropCounts, ArrayLength(subpropCounts),
> +		sizeof(subpropCounts[0]), SortPropertyAndCount);

Please align the second line such that the 's' in sizeof is below the '&'.
(Assignee)

Comment 8

5 years ago
Created attachment 613800 [details] [diff] [review]
qsort() instead of NS_QuickSort() in common cases

done.
btw I red "Mozilla Code styling" and didn't saw this moment with parameters alignment :) Is it so obvious? ;)
Attachment #613123 - Attachment is obsolete: true
Attachment #613800 - Flags: review?(benjamin)
Attachment #613800 - Flags: review?(benjamin) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Attachment #613800 - Flags: approval-mozilla-central?
Comment on attachment 613800 [details] [diff] [review]
qsort() instead of NS_QuickSort() in common cases

we think this could wait for 15
Attachment #613800 - Flags: approval-mozilla-central? → approval-mozilla-central-
FYI, toolkit/components/url-classifier/Entries.h doesn't exist since bug 673470 was backed out. I added a note to that bug about it and will land this patch without that change when the tree re-opens.
Actually, this doesn't build on Try.

../../../../toolkit/components/filepicker/nsFileView.cpp: In member function 'void nsFileView::SortArray(nsISupportsArray*)':
../../../../toolkit/components/filepicker/nsFileView.cpp:1000: error: invalid conversion from 'int (*)(const void*, const void*)' to 'int (*)(const void*, const void*, void*)'
../../../../toolkit/components/filepicker/nsFileView.cpp:1003: error: invalid conversion from 'int (*)(const void*, const void*)' to 'int (*)(const void*, const void*, void*)'
../../../../toolkit/components/filepicker/nsFileView.cpp:1006: error: invalid conversion from 'int (*)(const void*, const void*)' to 'int (*)(const void*, const void*, void*)'
../../../../toolkit/components/filepicker/nsFileView.cpp:1022: error: invalid conversion from 'int (*)(const void*, const void*, void*)' to 'int (*)(const void*, const void*)'
Should have noted that this was a b2g build that failed, which is gcc 4.4.3 I believe.

Updated

5 years ago
Status: REOPENED → NEW

Updated

5 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
>FYI, toolkit/components/url-classifier/Entries.h doesn't exist since bug 673470 
>was backed out. I added a note to that bug about it and will land this patch 
>without that change when the tree re-opens.

This bug doesn't seem to have made it to a landing, but I put a fix in bug 673470 for using qsort:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9ae0fe8297
https://hg.mozilla.org/mozilla-central/rev/ba9ae0fe8297
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 15

4 years ago
I also do not understand this bug. The description was to replace qsort with NS_QuickSort. Why is the patch the other way round? What is the proper thing to do here?
See Also: → bug 1141431
You need to log in before you can comment on or make changes to this bug.