Page MenuHomeFreeBSD

Add qsort_s(3)
ClosedPublic

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

Details

Summary

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

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 28705
Build 26722: arc lint + arc unit

Event Timeline

trasz created this revision.Jan 14 2020, 4:30 PM
trasz planned changes to this revision.Jan 14 2020, 4:47 PM

(Note that the tests are still TBD.)

kib added inline comments.Jan 14 2020, 6:04 PM
include/stdlib.h
348

Prototype is wrong. Take a look at the K.3.6.3.2.

349

Indent is wrong.

lib/libc/stdlib/Symbol.map
123

Why did you put the symbol under 1.5 version ?

rpokala added inline comments.
lib/libc/stdlib/qsort.3
270

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.)

337

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

arichardson added inline comments.Jan 15 2020, 10:04 AM
lib/libc/stdlib/qsort.3
337

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.

lib/libc/stdlib/qsort.c
99

void* context is the last argument for qsort_s

trasz updated this revision to Diff 66855.Jan 16 2020, 7:42 PM
trasz marked 4 inline comments as done.

Fix stuff, add tests.

trasz added inline comments.Jan 16 2020, 7:43 PM
lib/libc/stdlib/Symbol.map
123

Where should it go instead?

arichardson accepted this revision.Jan 16 2020, 11:09 PM

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

include/stdlib.h
327

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.

lib/libc/tests/stdlib/qsort_s_test.c
76

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
trasz updated this revision to Diff 66896.Jan 17 2020, 10:37 AM

Undo the mess caused by 'svn cp'.

This revision now requires review to proceed.Jan 17 2020, 10:37 AM
trasz added inline comments.Jan 17 2020, 10:39 AM
lib/libc/tests/stdlib/qsort_s_test.c
76

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

arichardson requested changes to this revision.Jan 17 2020, 10:39 AM
arichardson added inline comments.
lib/libc/stdlib/Makefile.inc
14–15

qsort_r.c is missing now.

This revision now requires changes to proceed.Jan 17 2020, 10:39 AM
arichardson added inline comments.Jan 17 2020, 10:40 AM
lib/libc/tests/stdlib/qsort_s_test.c
76

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

trasz updated this revision to Diff 66905.Jan 17 2020, 12:14 PM

Sigh, removed qsort_r from the wrong makefile.

arichardson added inline comments.Jan 17 2020, 12:31 PM
lib/libc/stdlib/Symbol.map
123

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

kib added inline comments.Jan 17 2020, 12:40 PM
lib/libc/stdlib/Makefile.inc
16

Why not merge this and previous line ?

lib/libc/stdlib/Symbol.map
123

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

trasz updated this revision to Diff 66906.Jan 17 2020, 1:16 PM

Fix symbol version, and formatting in the makefile.

trasz marked 5 inline comments as done.Jan 17 2020, 1:18 PM
trasz added inline comments.
lib/libc/stdlib/Symbol.map
123

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

kib accepted this revision.Jan 17 2020, 1:58 PM
arichardson accepted this revision.Jan 17 2020, 2:50 PM
This revision is now accepted and ready to land.Jan 17 2020, 2:50 PM
jilles added a subscriber: jilles.Jan 17 2020, 7:27 PM

Hmm, https://reviews.freebsd.org/D17083 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, https://reviews.freebsd.org/D17083 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.