glob: Add blocks support
Needs ReviewPublic
Actions

Authored by bnovkov on Fri, May 23, 2:39 PM.

Details

Reviewers

theraven
des

Group Reviewers

Klara

Summary

This change introduces the glob_b function which takes a block instead
of a function pointer.

Relnotes: yes
Sponsored by: Klara, Inc.
Obtained from: https://github.com/apple-oss-distributions/Libc

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 64376
Build 61260: arc lint + arc unit

Event Timeline

bnovkov created this revision.Fri, May 23, 2:39 PM

Herald added a subscriber: imp. · View Herald TranscriptFri, May 23, 2:39 PM

bnovkov requested review of this revision.Fri, May 23, 2:39 PM

Harbormaster completed remote builds in B64376: Diff 155918.Fri, May 23, 2:39 PM

bnovkov added a child revision: D50486: glob: Add tests for error callback functions and blocks.Fri, May 23, 2:39 PM

David historically has views on blocks, lets solicit his opinion as well.

include/glob.h
54	Is there any harm in just making this a single member union for !BLOCKS? It might be a little less ugly, at least.

I'm not quite sure what this is doing, but it isn't following the pattern that the rest of libc uses. We have DECLARE_BLOCK to declare a block type in a compiler-agnostic way and then CALL_BLOCK to call it. You don't need most of the #ifdef __BLOCKS__ bits at all, they're all handled by the abstraction layer.

See block_abi.h.

If I understand the API of glob[_b] correctly, the block is not captured after the call, it is simply passed down and invoked. If so, this is *all* that is needed, since you don't need any of the memory-management bits that complicated fts_b.

include/glob.h
54	I presume we want to support blocks in the caller even if libc is compiled without blocks support? In that case, this should probably have a `void*` member.
101	Why is this an error? Is this just a discriminator for the discriminated union?
lib/libc/gen/glob.c
261	What is this doing?
269	This doesn't follow the pattern for other libc things that support blocks. They are compiled unconditionally and use the macros if built with a compiler that doesn't support blocks.
1117	This should use the call-block macro if the compiler for libc does not support blocks.

This is also missing an update to the man page. The HISTORY bit should mention that glob_b first appeared in OS X 10.6.

Address @theraven 's comments:

Replace ifdef blocks with the corresponding block_api.h macros
Include missing manpage updates

Herald added a subscriber: ziaee. · View Herald TranscriptMon, May 26, 10:50 AM

In D50485#1152334, @theraven wrote:

I'm not quite sure what this is doing, but it isn't following the pattern that the rest of libc uses. We have DECLARE_BLOCK to declare a block type in a compiler-agnostic way and then CALL_BLOCK to call it. You don't need most of the #ifdef __BLOCKS__ bits at all, they're all handled by the abstraction layer.

See block_abi.h.

Thank you for the feedback, I managed to miss the block_abi.h header. I've updated the diff to use the block_abi.h macros and removed the unnecessary ifdef blocks.

bnovkov marked 4 inline comments as done.Mon, May 26, 10:59 AM

bnovkov added inline comments.

include/glob.h
101	Yes, this is just a union discriminator.
lib/libc/gen/glob.c
261	This current `glob` implementation starts by populating the `gl_errfunc` and `gl_flags` members before doing any actual matching, so this snippet was hoisted during the refactor. This makes it possible for `glob_b` to set the union discriminator and populate `gl_errblk`.

LGTM, assuming I understand the memory management correctly. I would be happier if something zeroed the gl_errblk field at the end of the call, because otherwise it is a dangling pointer. I don't *think* there's any code path that can reach it, but it's good practice to ensure that any pointer is either to a valid object or null.