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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Stefan Beller <sbeller@xxxxxxxxxx> 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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux