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.
Details
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
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? |
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. |
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 | |
175 | Please start new sentences on new lines: instead of a function pointer. | |
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 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 (No starting "The". Wording might need work. "block-oriented"?) |
Attempt to clean up the markup issues pointed by Warren.
I found it better to use "where" instead of "except".
lib/libc/gen/fts.3 | ||
---|---|---|
533 | This line should be deleted. There is no leading "The" in this form: "blahblah 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. |
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. |
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)
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: |