Page MenuHomeFreeBSD

Add qsort_s(3)

Authored by trasz on Jan 14 2020, 4:30 PM.



Add qsort_s(3). Apart from the constraints, it also makes it easier
to port software written for Linux variant of qsort_r(3).

Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

(Note that the tests are still TBD.)

343 ↗(On Diff #66743)

Prototype is wrong. Take a look at the K.

344 ↗(On Diff #66743)

Indent is wrong.

123 ↗(On Diff #66743)

Why did you put the symbol under 1.5 version ?

rpokala added inline comments.
270 ↗(On Diff #66743)

May suggest some wordsmithing, for English readability?

The qsort_s function behaves the same as qsort_r, except that

  • The order of arguments to compar is different
  • If nmemb or size are greater than RSIZE_MAX, or nmemb is non-zero and compar is NULL, then the runtime-constraint handler is called, and qsort_s returns an error. Note that the handler is called before qsort_s returns the error, and that the handler itself does not return.

(Assuming I interpreted the existing wording correctly; it's a little hard to follow, which is why I'm suggesting the re-wording.)

328 ↗(On Diff #66743)

If the glibc version of qsort_r() is using non-standard ordering, perhaps note that? i.e.

and the non-standard GNU libc implementation of

328 ↗(On Diff #66743)

I wouldn't necessarily write that GNU is non-standard since the real problem is that there is no standard for qsort_r... I guess glibc just did whatever seemed easiest for their qsort.

98 ↗(On Diff #66743)

void* context is the last argument for qsort_s

trasz marked 4 inline comments as done.

Fix stuff, add tests.

123 ↗(On Diff #66743)

Where should it go instead?

Looks fine to me but someone else should give the final approval.

327 ↗(On Diff #66855)

Unrelated to this patch, but all other types use _FOO_DECLARED. Would be nice if we could be consistent.
Annoyingly, the clang builtin headers use _RSIZE_T and _SIZE_T, etc. so they are not compatible.

76 ↗(On Diff #66855)

Should this check for == EINVAL instead of != 0? And the same for the other tests.

This revision is now accepted and ready to land.Jan 16 2020, 11:09 PM

Undo the mess caused by 'svn cp'.

This revision now requires review to proceed.Jan 17 2020, 10:37 AM
76 ↗(On Diff #66855)

I don't think so - the standard doesn't seem to define the actual error to return?

arichardson added inline comments.
14 ↗(On Diff #66896)

qsort_r.c is missing now.

This revision now requires changes to proceed.Jan 17 2020, 10:39 AM
76 ↗(On Diff #66855)

True, I don't think we need to be testing the specific implementation here so we could theoretically reuse the test for other implementations.

Sigh, removed qsort_r from the wrong makefile.

123 ↗(On Diff #66743)

I think looking at the other files 1.6 is the version for 13?

16 ↗(On Diff #66905)

Why not merge this and previous line ?

123 ↗(On Diff #66743)

To the current version for CURRENT-13, which is 1.6.

Fix symbol version, and formatting in the makefile.

trasz added inline comments.
123 ↗(On Diff #66743)

Ah, now I get it. I only looked at this particular

This revision is now accepted and ready to land.Jan 17 2020, 2:50 PM

Hmm, proposes to change qsort_r() argument order to glibc's since that is likely to become POSIX standard and is slightly better than our order.

This revision was automatically updated to reflect the committed changes.
trasz marked an inline comment as done.

Hmm, proposes to change qsort_r() argument order to glibc's since that is likely to become POSIX standard and is slightly better than our order.

True. Implementing qsort_s(3) is much easier compatibility-wise, though.