On Mon, May 15, 2017 at 11:26 AM, Jonathan Tan <jonathanta...@google.com> wrote:
> "erroneous"? yep, words are hard. > > I also don't understand the meaning of this paragraph - if you mean that > this patch teaches other callers to hardcode the sign, I don't see any such > changes in the diff below. The last two hunks of the patch switch two callers that call with a sign that is hard to reason about. > After reading the patch below, would this commit message be better: > > [begin] > diff.c: teach emit_line_0 to accept sign parameter > > Instead of a separate "first" parameter representing the first character of > the line to be printed, make emit_line_0 take an optional "sign" parameter > specifically intended to hold the sign of the line. Callers that store the > sign and line separately can use the "sign" parameter like they used the > "first" parameter previously, and callers that store the sign and line > together (or do not have a sign) no longer need to manipulate their > arguments to fit the requirements of emit_line_0. > > (And then mention that you have checked all the callers and that none of > them send '\n' or '\r' as the sign, as you have done in this version.) > [end] That describes the situation better, indeed. >> @@ -556,7 +547,7 @@ static void emit_line_0(struct diff_options *o, const >> char *set, const char *res >> static void emit_line(struct diff_options *o, const char *set, const char >> *reset, >> const char *line, int len) >> { >> - emit_line_0(o, set, reset, line[0], line+1, len-1); >> + emit_line_0(o, set, reset, 0, line, len); >> } > > > Maybe this function is unnecessary now that emit_line_0 can take the line > directly. That would produce a lot of code churn, specifically in later patches; but I can remove that function if anyone feels strongly about it. >> + char term[2]; >> + term[0] = options->line_termination; >> + term[1] = '\0'; >> + >> + emit_line(options, NULL, NULL, >> + term, 1); > > > If options->line_termination is 0, this is actually a zero-length string > (not 1). So passing in !!options->line_termination should be fine? Thanks, Stefan