Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS
- From
Jeff King <peff@peff.net>
- Date
- Oct 29, 2018, 14:25 UTC
- Message-ID
- <20181029142536.GE17668@sigill.intra.peff.net>
- In-Reply-To
- <xmqqo9bd8pko.fsf@gitster-ct.c.googlers.com>
On Mon, Oct 29, 2018 at 11:16:39AM +0900, Junio C Hamano wrote:
Show 29 quoted lines
> Jeff King <peff@peff.net> writes: > > > 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. > > True, but the way "grep" code works with threads is already strange, > and I suspect that the subtle strangeness you feel mostly comes from > that. The single threaded code signaled "hit" with return value of > individual functions like grep_file(), but the meaning of return > value from them is changed to "does not matter--we do not have > meaningful result yet at this point" when threading is used. > > In the new world order where "threading is the norm but > single-threaded is still supported", I wonder if we would want to > drive the single threaded case the same way with the add_work(run) > interface and return the "did we see hits?" etc. from the same (or > at lesat "more similar than today's") interface, so that the flow of > data smells the same in both cases.
Right, your second paragraph here is a better statement of what I was trying to get at. ;)
But if the problem is simply that we are not quite there yet in the grep code, I am OK with taking this as the first pass, and knowing that there is more cleanup to be done later (though that sort of thing is IMHO very useful in a commit message).
-Peff