Re: [PATCH 03/19] diff.c: drop 'nofirst' from emit_line_0
- Date: Mon, 15 May 2017 11:33:03 -0700
- From: Stefan Beller <sbeller@xxxxxxxxxx>
- Subject: 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:
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:
> 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.)
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
>> const char *line, int len)
>> - emit_line_0(o, set, reset, line, 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
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;
>> + term = options->line_termination;
>> + term = '\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?