On Fri, Jun 30, 2017 at 2:11 PM, Junio C Hamano <[email protected]> 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.
>
>> +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.
>
> 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
