protect unistd.h inclusion with HAVE_UNISTD_H #339
Conversation
|
@swift-ci please test |
dispatch/dispatch.h
Outdated
| @@ -35,7 +35,7 @@ | |||
| #include <stdint.h> | |||
| #include <stdbool.h> | |||
| #include <stdarg.h> | |||
| #if !defined(HAVE_UNISTD_H) || HAVE_UNISTD_H | |||
| #if HAVE_UNISTD_H | |||
There was a problem hiding this comment.
this doesn't work in public headers sadly because HAVE_UNISTD_H is a configure option, so doing that will break unix because HAVE_UNISTD_H is not defined.
there are two options, either use __has_include() but that requires clang support for the library user which I'm not a fan of (while requiring clang to build dispatch is a thing, requiring clang to build clients is another).
However, a thing we've started to do on the darwin side is to unifdef headers that we install.
I would be more than willing to accept a patch like that if the cmake build system is able to unifdef installed headers, and when installing on windows you'd unifdef -DHAVE_UNISTD_H=0 and on unix unifdef -DHAVE_UNISTD_H=1 and done.
tests/dispatch_group.c
Outdated
| @@ -19,7 +19,9 @@ | |||
| */ | |||
|
|
|||
| #include <dispatch/dispatch.h> | |||
| #if HAVE_UNISTD_H | |||
| #include <unistd.h> | |||
There was a problem hiding this comment.
for all of these because <dispatch/dispatch.h> will do the right thing we should just remove the #include instead
There was a problem hiding this comment.
typically nowadays (not merged back to github yet) Darwin does this to headers:
unifdef -U__DISPATCH_BUILDING_DISPATCH__ -U__linux__ -DTARGET_OS_WIN32=0 -U__ANDROID__
it allows to cleanup the headers from stuff that makes no sense on darwin to clean them up, I think we should do the same for various platforms to that headers for said plaforms are decluttered from stuff that makes no sense on said platform.
but we can't have '#if HAVE_*' on public/installed headers as we don't want to ship a <dispatch/config.h> (Darwin certainly doesn't so that change as is would break darwin).
There was a problem hiding this comment.
Hmm, another option is to use the compiler instead. unistd.h is only available on Unix and macOS X. We could also use:
#if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__))
I think that adding the hooks for the unifdef should be done anyways as @das mentioned there was interest in unifying the macOS build into the CMake system as well, but I need to look into how to do that nicely (so I think that doing that a bit later would be nice).
This ensures that all the inclusions of unistd.h inclusion are protected with the check that the header exists. This header is only available on unix like environments to define the standard of compliance. This protection is needed for other environments like Windows.
|
@swift-ci please test |
|
@MadCoder okay with this? Once this is done, I'll update the other patch with more fixes :-) |
protect unistd.h inclusion with HAVE_UNISTD_H Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>
This ensures that all the inclusions of unistd.h inclusion are protected
with the check that the header exists. This header is only available on
unix like environments to define the standard of compliance. This
protection is needed for other environments like Windows.