On Sat, Oct 27, 2018 at 09:09:59AM +0200, Nguyễn Thái Ngọc Duy wrote:
> diff --git a/builtin/grep.c b/builtin/grep.c
> index d8508ddf79..29221e1003 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -34,7 +34,6 @@ static int recurse_submodules;
> #define GREP_NUM_THREADS_DEFAULT 8
> static int num_threads;
>
> -#ifndef NO_PTHREADS
> static pthread_t *threads;
>
> /* We use one producer thread and THREADS consumer
> @@ -265,13 +264,6 @@ static int wait_all(void)
>
> return hit;
> }
> -#else /* !NO_PTHREADS */
> -
> -static int wait_all(void)
> -{
> - return 0;
> -}
> -#endif
Cases like this are kind of weird. I'd expect to see wait_all() return
immediately when !HAVE_THREADS. But we don't need to, because later we
do this:
> - if (num_threads)
> + if (HAVE_THREADS && num_threads)
> hit |= wait_all();
Which I think works, but it feels like we're introducing some subtle
dependencies about which functions get called in which cases. I'd hoped
in general that the non-threaded code paths could mostly just collapse
down into the same as the "threads == 1" cases, and we wouldn't have to
ask "are threads even supported" in a lot of places.
I dunno. My comment isn't really an objection exactly, but mostly just
an indication that I'm still thinking through what the best approach is,
and what end state we want to end up in.
-Peff
