Re: [PATCHv2 22/25] diff.c: color moved lines differently
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 30, 2017, 21:11 UTC
- Message-ID
- <xmqq1sq1qiyg.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170630205310.7380-23-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 16 quoted lines
> +static int next_byte(const char **cp, const char **endp,
> + const struct diff_options *diffopt)
> +{
> + int retval;
> +
> + if (*cp > *endp)
> + return -1;
> +
> + if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_CHANGE)) {
> + while (*cp < *endp && isspace(**cp))
> + (*cp)++;
> + /*
> + * After skipping a couple of whitespaces, we still have to
> + * account for one space.
> + */
> + return (int)' ';Do we need a cast here?
Show 10 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?
It does not make a difference in correctness; two lines that differ only by their trailing whitespaces will be hashed into the same bin even when we are not using ignore-whitespace-at-eol, making the hashmap a bit less efficient than necessary.
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:
Show 7 quoted lines
> + 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.
Show 5 quoted lines
> +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.