Page MenuHomeFreeBSD

More Apple-block function support
AbandonedPublic

Authored by pfg on Jun 20 2015, 3:30 PM.

Details

Reviewers
theraven
wblock
sson
brooks
Group Reviewers
manpages
Summary

I found this implementation of some missing block functions
in Stacey Son's repo.
I only did some minimal changes and added the functions to the
Symbol map. I haven't done any testing but I thought it would be
good to make them more visible as we should try to get the
support for the block functions and GCD workqueues in
FreeBSD 11.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pfg updated this revision to Diff 6351.Jun 20 2015, 3:30 PM
pfg retitled this revision from to More Apple-block function support.
pfg updated this object.
pfg edited the test plan for this revision. (Show Details)
pfg added reviewers: theraven, sson.
theraven edited edge metadata.Jun 20 2015, 4:22 PM

Comments inline.

lib/libc/gen/err.c
85

This doesn't look right. Will it pass the block as the argument when err_exit is called? If not, then bound variables in the block will be undefined values.

lib/libc/gen/fts.c
62

We should move these declarations into block_abi.h and not have them copied into every file that uses them. We should also use __weak_symbol, rather than the expanded form of the attribute.

328

Please can we have an assert for _Block_release != 0 if sp->fts_compar_b != NULL? This will only happen if _Block_copy exists but _Block_release doesn't, which means that something's badly wrong. Ideally, we'd do the _Block_release != 0 check up on line 261, so we never get into this situation in the first place.

1063

Is there a reason not to use qsort_b here?

pfg added inline comments.Jun 20 2015, 9:46 PM
lib/libc/gen/fts.c
62

I will move them. FWIW, I changed them to the expanded form as a reminder that we still don't have __weak_symbol. Currently __weak is still the only option in cdefs.h.

pfg updated this revision to Diff 6354.Jun 20 2015, 9:49 PM
pfg edited edge metadata.

Move prototypes to the header.
The rest I leave to sson :).

wblock added a subscriber: wblock.Jun 21 2015, 1:50 PM
wblock added inline comments.
lib/libc/gen/err.3
173

This should use the function markup, should probably have a comma after the function name for a pause:

function is like
.Fn err_set_exit ,
except it takes the block pointer,

175

Please start new sentences on new lines:

instead of a function pointer.
Note that the

lib/libc/gen/fts.3
69

s/returns/return/

539

This sentence is ambiguous here. The "instead of" can be misinterpreted. It might not be necessary to describe what fts_open does in this sentence, just what fts_open_b does. That reduces the number of things that can be confused. A suggestion follows. Note that I also find the "The blahblah function" needlessly redundant, at least after the first use. I suggest just referring to these and other special words by their name alone, like this sentence refers to fts_open.

.Fn fts_open_b
is like
.Fn fts_open ,
except
.Fa compar
is a block pointer that is passed to
.Xr qsort_b 3 .

A possibly better way to do this would be to say (markup omitted) "fts_open_b is a block-based version of fts_open. It works in the same way, but compar is a block pointer that is passed to qsort_b(3)."

lib/libc/gen/glob.3
335

As above, it might be better to just state the difference and not what the other function does. It might also be better as two sentences, or at least with a comma. In other words:

.Fn glob_b
is a block-based version of
.Fn glob .
The error callback is the block pointer
.Fa errblock .

(No starting "The". Wording might need work. "block-oriented"?)

pfg updated this revision to Diff 6371.Jun 22 2015, 12:27 AM
pfg marked 2 inline comments as done.

Attempt to clean up the markup issues pointed by Warren.

I found it better to use "where" instead of "except".

wblock added inline comments.Jun 22 2015, 2:02 PM
lib/libc/gen/fts.3
533

This line should be deleted. There is no leading "The" in this form:

"blahblah is"
as opposed to
"The blahblah function is"

541

This last part is still a little ambiguous, although I can see where it might be wanted for comparison. The sentence could be ended immediately after .Xr qsort_b 3. Given the clarity improvement to the first part of the sentence, this part should work either way.

pfg updated this revision to Diff 6389.Jun 23 2015, 5:11 AM
pfg marked 3 inline comments as done.

More polishing by Warren (thanks!).

wblock accepted this revision.Jun 23 2015, 3:32 PM
wblock added a reviewer: wblock.

Looks good for the man pages. Still worth checking with igor and mandoc -Tlint.

This revision is now accepted and ready to land.Jun 23 2015, 3:32 PM
pfg updated this revision to Diff 6439.Jun 24 2015, 6:37 PM
pfg edited edge metadata.
pfg removed rS FreeBSD src repository as the repository for this revision.

__weak_symbol exists now.

This revision now requires review to proceed.Jun 24 2015, 6:37 PM

Phabricator is still somewhat confused, and shows "Loading..." on some things that probably should have been reviewed.

lib/libc/gen/err.3
31

This should be the date this was changed/added in FreeBSD, unless there is something I've missed. Likewise in the other changed or added man pages.

168

Better to just split the sentence in two:

to perform any necessary cleanup.
Passing a null function pointer for

pfg updated this revision to Diff 6442.Jun 24 2015, 7:32 PM
pfg edited edge metadata.

Note that this is still not fixed, wrt to David's comments so we don't have date for the manpage changes.
(I am also having issues with Phabricator so I am uploading patches)

pfg updated this revision to Diff 6514.Jun 27 2015, 7:51 PM

Fix build issue with __weak_symbol unknown in block_abi.h

pfg abandoned this revision.Apr 27 2019, 3:41 PM
pfg added a reviewer: brooks.

This has rusted since the last revision: fts had some changes (mostly just updated prototypes) and glob has been rewritten.
It needs some fresh eyes into it (cc'ing brooks in case he wants to take over).

lib/libc/include/block_abi.h
69

buildworld fails with:
...

> lib/libc_nonshared (obj,depend,all,install)

In file included from /scratch/tmp/pfg/head/lib/libc/stdlib/qsort_r.c:7:
/scratch/tmp/pfg/head/lib/libc/include/block_abi.h:68: error: expected '=', ',', ';', 'asm' or 'attribute' before 'void'
/scratch/tmp/pfg/head/lib/libc/include/block_abi.h:69: error: expected '=', ',', ';', 'asm' or 'attribute' before 'void'
...