Re: [PATCHv5 04/17] diff: introduce more flexible emit function

From: Stefan Beller <sbeller@google.com>
Date: Jun 13, 2017, 23:41 UTC
Message-ID: <CAGZ79kY9ByaHe_fx+S3neT6+1LcA_Q13bbVha8AKHUUO32FgeA@mail.gmail.com>
In-Reply-To: <20170613145457.2e888c27@twelve2.svl.corp.google.com>

On Tue, Jun 13, 2017 at 2:54 PM, Jonathan Tan <jonathantanmy@google.com> wrote:

Show 12 quoted lines

> On Wed, 24 May 2017 14:40:23 -0700
> Stefan Beller <sbeller@google.com> wrote:
>
>> Currently, diff output is written either through the emit_line_0
>> function or through the FILE * in struct diff_options directly. To
>> make it easier to teach diff to buffer its output (which will be done
>> in a subsequent commit), introduce a more flexible emit_line() function.
>> In this commit, direct usages of emit_line_0() are replaced with
>> emit_line(); subsequent commits will also replace usages of the
>> FILE * with emit().
>
> Check the names of the functions in this paragraph.

Only the very last word needed replacement of /s/emit/emit_line/.

Show 16 quoted lines

>
>> diff --git a/diff.c b/diff.c
>> index 2f9722b382..3569857818 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -516,36 +516,30 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
>>       ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
>>  }
>>
>> -static void emit_line_0(struct diff_options *o, const char *set, const char *reset,
>> -                     int first, const char *line, int len)
>> +static void emit_line(struct diff_options *o, const char *set, const char *reset,
>> +                   int add_line_prefix, int sign, const char *line, int len)
>
> In the future, this function is going to be used even to emit partial
> lines

Yes.

> - could this be called emit() instead?

Despite having good IDEs available some (including me) very much like working with raw text, and then having a function named as a common string doesn't help.

After this patch

  $ git grep emit_line |wc -l
  16
  # not all are this function, there is
  emit_line_checked as well. But 16 is not too much.

But if renamed to emit():

  $ git grep emit -- diff.c |wc -l
  60

You could argue I'd just have to grep for "emit (" instead, but that then I would have rely on correct whitespacing or use a regex already. Complexity which I would not like.

So I am not sure if this is helping a reader. (Not the casual reader, but the one grepping for this function)

Maybe we can settle on a different name though, such as emit_string which is not a prefix of a dozen different other functions?

Thanks, Stefan