Stefan Beller <sbel...@google.com> writes:

> +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?

> +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:

> +             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.

Reply via email to