Re: [PATCH] simple-ipc: correct ifdefs when NO_PTHREADS is defined
- From
- Jeff Hostetler <git@jeffhostetler.com>
- Date
- May 19, 2021, 14:29 UTC
- Message-ID
- <79bf42e7-3923-a901-53eb-1aac13c53e6b@jeffhostetler.com>
- In-Reply-To
- <xmqqk0nv1rc4.fsf@gitster.g>
On 5/18/21 8:48 PM, Junio C Hamano wrote:
Show 64 quoted lines
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> +# Simple-ipc requires threads and platform-specific IPC support. >> +# (We group all Unix variants in the top-level else because Windows >> +# also defines NO_UNIX_SOCKETS.) >> ifdef USE_WIN32_IPC >> + BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC >> LIB_OBJS += compat/simple-ipc/ipc-shared.o >> LIB_OBJS += compat/simple-ipc/ipc-win32.o >> +else >> +ifndef NO_PTHREADS >> +ifndef NO_UNIX_SOCKETS >> + BASIC_CFLAGS += -DSUPPORTS_SIMPLE_IPC >> + LIB_OBJS += compat/simple-ipc/ipc-shared.o >> + LIB_OBJS += compat/simple-ipc/ipc-unix-socket.o >> +endif >> +endif > > OK, so "!defined(NO_PTHREADS) && !defined(NO_UNIX_SOCKETS)" is the > requirement for SIMPLE_IPC unless you are on Windows. > >> diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c >> index 38689b278df3..c23b17973983 100644 >> --- a/compat/simple-ipc/ipc-unix-socket.c >> +++ b/compat/simple-ipc/ipc-unix-socket.c >> @@ -6,10 +6,16 @@ >> #include "unix-socket.h" >> #include "unix-stream-server.h" >> >> +#ifdef SUPPORTS_SIMPLE_IPC >> + >> #ifdef NO_UNIX_SOCKETS >> #error compat/simple-ipc/ipc-unix-socket.c requires Unix sockets >> #endif >> >> +#ifdef NO_PTHREADS >> +#error compat/simple-ipc/ipc-unix-socket.c requires pthreads >> +#endif >> + > > Do we want to duplicate the requirement here and risk them drifting > apart? > >> @@ -997,3 +1003,5 @@ void ipc_server_free(struct ipc_server_data *server_data) >> free(server_data->fifo_fds); >> free(server_data); >> } >> + >> +#endif /* SUPPORTS_SIMPLE_IPC */ > > If anything, I do not think we want a huge #ifdef/#endif around the > whole file. Feeding this source to your compiler when these three C > proprocessor macros do not agree with its use is an error, so perhaps > lose all of these #ifdef/#endif around the three macros and refer human > readers to the top-level Makefile with a comment, e.g. > > /* > * Non Windows platforms need !NO_UNIX_SOCKETS and !NO_PTHREADS > * to compile and use this file. See the top-level Makefile. > */ > > if we really wanted to have a way to help builders identify the > reason why their build is failing. >
Would it be better to just have something like the following at the top of the source files and leave the details to the Makefile:
#ifndef SUPPORTS_SIMPLE_IPC /* * This source file should only be included when Simple IPC * is supported. See the top-level Makefile. */ #error SUPPORTS_SIMPLE_IPC not defined #endif