Re: [PATCH 1/4] Start to replace locate_rename_dst() with a generic function.

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 4, 2010, 20:45 UTC
Message-ID: <20101104204555.GA18187@burratino>
In-Reply-To: <1288902795-8597-2-git-send-email-ydirson@altern.org>

Yann Dirson wrote:

Show 10 quoted lines

> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -6,6 +6,10 @@
>  #include "diffcore.h"
>  #include "hash.h"
>  
> +#define locate_element(list,elem,insert_ok)			\
> +	_locate_element(elem, &list##_nr, &list##_alloc,	\
> +			insert_ok)
> +

Is this syntactic sugar needed?

 static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,
						  insert_ok)
 {
	return locate_element(&rename_dst_nr, &rename_dst_alloc, elem, insert_ok);
 }

takes more advantage of the compiler's typechecking and looks easy enough to read.

Since this is local to diffcore-rename, I don't mind the locate_element() name, but if this is to be used more widely I think it would need to be named more precisely. (find_or_insert_in_array()?)

> @@ -13,14 +17,17 @@ static struct diff_rename_dst {

[...]

Show 11 quoted lines

> -static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,
> -						 int insert_ok)
> +static struct diff_rename_dst *_locate_element(struct diff_filespec *two,
> +					       int *elem_nr_p, int *elem_alloc_p,
> +					       int insert_ok)
>  {
>  	int first, last;
>  
>  	first = 0;
> -	last = rename_dst_nr;
> +	last = (*elem_nr_p);

I guess these parentheses came from search+replace? It's more readable without them.

[...]

> +	(*elem_nr_p)++;

Except for this one.

Generally, the approach seems sane so far.