Re: [PATCH] simple-ipc: correct ifdefs when NO_PTHREADS is defined

From: Junio C Hamano <gitster@pobox.com>
Date: May 19, 2021, 00:48 UTC
Message-ID: <xmqqk0nv1rc4.fsf@gitster.g>
In-Reply-To: <pull.955.git.1621352192238.gitgitgadget@gmail.com>

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 15 quoted lines

> +# 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.

Show 18 quoted lines

> 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?

Show 6 quoted lines

> @@ -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.