Re: [PATCH 7/7] xdiff: remove xdl_free(), use free() instead

From: Phillip Wood <phillip.wood123@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH 7/7] xdiff: remove xdl_free(), use free() instead
Date: Fri, 8 Jul 2022 18:51:43 +0100	[thread overview]
Message-ID: <6b8fc0e8-c446-1275-12f3-e6520de9584d@gmail.com> (raw)
In-Reply-To: <patch-7.7-a1bf9a94f0a-20220708T140354Z-avarab@gmail.com>

Hi Ævar

On 08/07/2022 15:20, Ævar Arnfjörð Bjarmason wrote:
> Remove the xdl_free() wrapper macro in favor of using free()
> directly. The wrapper macro was brought in with the initial import of
> xdiff/ in 3443546f6ef (Use a *real* built-in diff generator,
> 2006-03-24).
> 
> As subsequent discussions on the topic[1] have made clear there's no
> reason to use this wrapper.

That link is to a message where you assert that there is no reason for 
the wrapper, you seem to be the only person in that thread making that 
argument. The libgit2 maintainer and others are arguing that it is worth 
keeping to make it easy to swap the allocator.

> Both git itself as well as any external
> users such as libgit2 compile the xdiff/* code as part of their own
> compilation, and can thus find the right malloc() and free() at
> link-time.

I'm struggling to see how that is simpler than the current situation 
with xdl_malloc(). Perhaps you could show how you think libgit2 could 
easily make xdiff use git__malloc() instead of malloc() without changing 
the malloc() calls (the message you linked to essentially suggests they 
do a search and replace). If people cannot swap in another allocator 
without changing the code then you are imposing a barrier to them 
contributing patches back to git's xdiff.

Best Wishes

Phillip

> When compiling git we already find a custom malloc() and free()
> e.g. if compiled with USE_NED_ALLOCATOR=YesPlease.
> 
> 1. https://lore.kernel.org/git/220415.867d7qbaad.gmgdl@evledraar.gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   xdiff/xdiff.h      |  3 ---
>   xdiff/xdiffi.c     |  4 ++--
>   xdiff/xhistogram.c |  6 +++---
>   xdiff/xpatience.c  |  8 ++++----
>   xdiff/xprepare.c   | 28 ++++++++++++++--------------
>   xdiff/xutils.c     |  2 +-
>   6 files changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index df048e0099b..a37d89fcdaf 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -118,9 +118,6 @@ typedef struct s_bdiffparam {
>   	long bsize;
>   } bdiffparam_t;
>   
> -
> -#define xdl_free(ptr) free(ptr)
> -
>   void *xdl_mmfile_first(mmfile_t *mmf, long *size);
>   long xdl_mmfile_size(mmfile_t *mmf);
>   
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 6590811634f..375bb81a8aa 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -359,7 +359,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   	res = xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,
>   			   kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0,
>   			   &xenv);
> -	xdl_free(kvd);
> +	free(kvd);
>   
>   	return res;
>   }
> @@ -960,7 +960,7 @@ void xdl_free_script(xdchange_t *xscr) {
>   
>   	while ((xch = xscr) != NULL) {
>   		xscr = xscr->next;
> -		xdl_free(xch);
> +		free(xch);
>   	}
>   }
>   
> diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c
> index f20592bfbdd..be35d9c9697 100644
> --- a/xdiff/xhistogram.c
> +++ b/xdiff/xhistogram.c
> @@ -240,9 +240,9 @@ static int fall_back_to_classic_diff(xpparam_t const *xpp, xdfenv_t *env,
>   
>   static inline void free_index(struct histindex *index)
>   {
> -	xdl_free(index->records);
> -	xdl_free(index->line_map);
> -	xdl_free(index->next_ptrs);
> +	free(index->records);
> +	free(index->line_map);
> +	free(index->next_ptrs);
>   	xdl_cha_free(&index->rcha);
>   }
>   
> diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c
> index bb328d9f852..8fffd2b8297 100644
> --- a/xdiff/xpatience.c
> +++ b/xdiff/xpatience.c
> @@ -233,7 +233,7 @@ static int find_longest_common_sequence(struct hashmap *map, struct entry **res)
>   	/* No common unique lines were found */
>   	if (!longest) {
>   		*res = NULL;
> -		xdl_free(sequence);
> +		free(sequence);
>   		return 0;
>   	}
>   
> @@ -245,7 +245,7 @@ static int find_longest_common_sequence(struct hashmap *map, struct entry **res)
>   		entry = entry->previous;
>   	}
>   	*res = entry;
> -	xdl_free(sequence);
> +	free(sequence);
>   	return 0;
>   }
>   
> @@ -358,7 +358,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,
>   			env->xdf1.rchg[line1++ - 1] = 1;
>   		while(count2--)
>   			env->xdf2.rchg[line2++ - 1] = 1;
> -		xdl_free(map.entries);
> +		free(map.entries);
>   		return 0;
>   	}
>   
> @@ -372,7 +372,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,
>   		result = fall_back_to_classic_diff(&map,
>   			line1, count1, line2, count2);
>    out:
> -	xdl_free(map.entries);
> +	free(map.entries);
>   	return result;
>   }
>   
> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index 4182d9e1c0a..169629761c0 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -89,7 +89,7 @@ static int xdl_init_classifier(xdlclassifier_t *cf, long size, long flags) {
>   
>   	GALLOC_ARRAY(cf->rcrecs, cf->alloc);
>   	if (!cf->rcrecs) {
> -		xdl_free(cf->rchash);
> +		free(cf->rchash);
>   		xdl_cha_free(&cf->ncha);
>   		return -1;
>   	}
> @@ -102,8 +102,8 @@ static int xdl_init_classifier(xdlclassifier_t *cf, long size, long flags) {
>   
>   static void xdl_free_classifier(xdlclassifier_t *cf) {
>   
> -	xdl_free(cf->rcrecs);
> -	xdl_free(cf->rchash);
> +	free(cf->rcrecs);
> +	free(cf->rchash);
>   	xdl_cha_free(&cf->ncha);
>   }
>   
> @@ -230,11 +230,11 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_
>   	return 0;
>   
>   abort:
> -	xdl_free(ha);
> -	xdl_free(rindex);
> -	xdl_free(rchg);
> -	xdl_free(rhash);
> -	xdl_free(recs);
> +	free(ha);
> +	free(rindex);
> +	free(rchg);
> +	free(rhash);
> +	free(recs);
>   	xdl_cha_free(&xdf->rcha);
>   	return -1;
>   }
> @@ -242,11 +242,11 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_
>   
>   static void xdl_free_ctx(xdfile_t *xdf) {
>   
> -	xdl_free(xdf->rhash);
> -	xdl_free(xdf->rindex);
> -	xdl_free(xdf->rchg - 1);
> -	xdl_free(xdf->ha);
> -	xdl_free(xdf->recs);
> +	free(xdf->rhash);
> +	free(xdf->rindex);
> +	free(xdf->rchg - 1);
> +	free(xdf->ha);
> +	free(xdf->recs);
>   	xdl_cha_free(&xdf->rcha);
>   }
>   
> @@ -424,7 +424,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>   	}
>   	xdf2->nreff = nreff;
>   
> -	xdl_free(dis);
> +	free(dis);
>   
>   	return 0;
>   }
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 865e08f0e93..00eeba452a5 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -88,7 +88,7 @@ void xdl_cha_free(chastore_t *cha) {
>   
>   	for (cur = cha->head; (tmp = cur) != NULL;) {
>   		cur = cur->next;
> -		xdl_free(tmp);
> +		free(tmp);
>   	}
>   }
>   