On Mon, Oct 26, 2015 at 10:25:41PM -0700, Victor Leschuk wrote: > >> @@ -22,6 +22,7 @@ SYNOPSIS > >> [--color[=<when>] | --no-color] > >> [--break] [--heading] [-p | --show-function] > >> [-A <post-context>] [-B <pre-context>] [-C <context>] > >> + [--threads <num>] > > > Is this the best place for this option? I know the current list isn't > > sorted in any particular way, but here you're splitting up the set of > > context options (`-A`, `-B`, `-C` and `-W`). > > Agree, I'll move the option both here and in documentation. > > >> -static int wait_all(void) > >> +static int wait_all(struct grep_opt *opt) > > > I'm not sure passing a grep_opt in here is the cleanest way to do this. > > Options are a UI concept and all we care about here is the number of > > threads. > > > Since `threads` is a global, shouldn't the number of threads be a global > > as well? Could we reuse `use_threads` here (possibly renaming it > > `num_threads`)? > > This thought also crossed my mind, however we already pass grep_opt to > start_threads() function, so I think passing it to wait_all() is not > that ugly, and kind of symmetric. And I do not like the idea of > duplicating same information in different places. What do you think?
The grep_opt in start_threads() is being passed through to run(), so it seems slightly different to me. If the threads were being setup in grep.c (as opposed to builtin/grep.c) then I'd agree that it belongs in grep_opt, but since this is local to this particular user of the grep infrastructure adding num_threads to the grep_opt structure at all feels wrong to me. Note that I wasn't suggesting passing num_threads as a parameter to wait_all(), but rather having it as global state that is accessed by wait_all() in the same way as the `threads` array. If we rename use_threads to num_threads and just use that, then we only have the information in one place don't we? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
