On Mon, May 15, 2017 at 11:26 AM, Jonathan Tan <[email protected]> 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
