Page MenuHomeFreeBSD

Add qsort_s(3)
ClosedPublic

Authored by trasz on Jan 14 2020, 4:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 8:20 PM
Unknown Object (File)
Wed, Nov 20, 12:25 AM
Unknown Object (File)
Sun, Nov 17, 10:01 AM
Unknown Object (File)
Sun, Nov 17, 9:40 AM
Unknown Object (File)
Sun, Nov 17, 9:26 AM
Unknown Object (File)
Sun, Nov 17, 9:23 AM
Unknown Object (File)
Sun, Nov 10, 2:07 PM
Unknown Object (File)
Fri, Nov 1, 11:39 PM
Subscribers

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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28666
Build 26692: arc lint + arc unit

Event Timeline

(Note that the tests are still TBD.)

include/stdlib.h
343

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

344

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

328

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

lib/libc/stdlib/qsort.3
328

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
98

void* context is the last argument for qsort_s

trasz marked 4 inline comments as done.

Fix stuff, add tests.

lib/libc/stdlib/Symbol.map
123

Where should it go instead?

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 ↗(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
lib/libc/tests/stdlib/qsort_s_test.c
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.
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
lib/libc/tests/stdlib/qsort_s_test.c
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.

lib/libc/stdlib/Symbol.map
123

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

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.

Fix symbol version, and formatting in the makefile.

trasz added inline comments.
lib/libc/stdlib/Symbol.map
123

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

This revision is now accepted and ready to land.Jan 17 2020, 2:50 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.