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

[Jonathan Nieder <jrnieder@gmail.com>]

Yann Dirson wrote:

> --- 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 {
[...]
> -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.