Re: [PATCHv2 22/25] diff.c: color moved lines differently

From: Stefan Beller <sbeller@google.com>
Date: Jun 30, 2017, 21:21 UTC
Message-ID: <CAGZ79kbD13RgcTuiegn4J1piFZrNu6bAxCGEKDYLThnVuYQLrg@mail.gmail.com>
In-Reply-To: <xmqq1sq1qiyg.fsf@gitster.mtv.corp.google.com>

On Fri, Jun 30, 2017 at 2:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> +             return (int)' ';
>
> Do we need a cast here?

No, I figured it is good to have it here explicitly, though. We can drop that if you have strong preferences one way or another.

Show 13 quoted lines

>
>> +static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_options *o)
>> +{
>> +     if (o->xdl_opts & XDF_WHITESPACE_FLAGS) {
>> +             static struct strbuf sb = STRBUF_INIT;
>> +             const char *ap = es->line, *ae = es->line + es->len;
>> +             int c;
>> +
>> +             strbuf_reset(&sb);
>> +             while (ae > ap && isspace(*ae))
>> +                     ae--;
>
> Not testing for the AT_EOL option here?

No, because the other options are stronger than the AT_EOL, such that as you note it is still correct.

If in the future, we'd have another new option e.g. IGNORE_TAB_BLANK_CONVERSION_BUT_WARN_ON_LENGTH_DIFF (useful for python programmers ;) this would break.

Show 39 quoted lines

>
> By the way, this is an unrelated tangent because I think you
> inherited this pattern by copying and pasting from elsewhere, but I
> think it would be better if we avoid casting the function pointer
> type like this:
>
>> +             if (o->color_moved) {
>> +                     struct hashmap add_lines, del_lines;
>> +
>> +                     hashmap_init(&del_lines,
>> +                                  (hashmap_cmp_fn)moved_entry_cmp, o, 0);
>> +                     hashmap_init(&add_lines,
>> +                                  (hashmap_cmp_fn)moved_entry_cmp, o, 0);
>
> When hashmap_cmp_fn's definition changes, these two calling sites
> won't be caught as passing a incorrectly typed callback function by
> the compiler.
>
> Instead, we can match the actual implementation of the callback
> function, e.g.
>
>> +static int moved_entry_cmp(const struct diff_options *diffopt,
>> +                        const struct moved_entry *a,
>> +                        const struct moved_entry *b,
>> +                        const void *keydata)
>> +{
>
> to the expected function type, i.e.
>
> static int moved_entry_cmp(const void *fndata,
>                            const void *entry, const void *entry_or_key,
>                            const void *keydata)
> {
>         const struct diff_options *diffopt = fndata;
>         const struct moved_entry *a = entry;
>         const struct moved_entry *b = entry_or_key;
>         ...
>
> by casting the parameters.

I agree. I can make a cleanup throughout the whole code base, but I would prefer if that is done in a separate series, as this is already slightly lengthy.

Thanks, Stefan