Web lists-archives.com

Re: [PATCH 03/19] diff.c: drop 'nofirst' from emit_line_0




On Mon, May 15, 2017 at 11:26 AM, Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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