Re: [PATCH v3 04/10] xdiff: delete xdl_get_rec() in xemit

[Junio C Hamano <gitster@pobox.com>]

"Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ezekiel Newren <ezekielnewren@gmail.com>
>
> This function aliases the fields of xrecord_t, which makes it harder
> to track the usages of those fields. Delete it.

Can we phrase it in a way that is a bit more friendly to casual
readers?  It is hard to tell if the function is serving any useful
purpose from the above.  If it is doing something useful, but is
hard to read the surrounding code, that wouldn't be a good reason to
remove it.

It seems that this patch does what people often call "inlining a
function call".  I am not sure your comment about aliasing or if
that is why the code is harder than necessary to read.

> -static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *ecb) {
> -	long size, psize = strlen(pre);
> -	char const *rec;
> -
> -	size = xdl_get_rec(xdf, ri, &rec);
> -	if (xdl_emit_diffrec(rec, size, pre, psize, ecb) < 0) {

So we used to get "rec" and "size" separately because xdl_get_rec()
made xrecord_t inaccessible to its callers.  To call a helper
function that takes a <ptr, len> pair, xdl_get_rec() is the way to
grab that <ptr, len> pair out of the record index.

> +static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *ecb)
> +{
> +	xrecord_t *rec = xdf->recs[ri];
>  
> +	if (xdl_emit_diffrec(rec->ptr, rec->size, pre, strlen(pre), ecb) < 0) {
>  		return -1;
>  	}

But by directly peeking into the xdf->recs[] array, we do not have
to.  Each element of the array is the <ptr, len> pair we want.

The updated code is certainly easier to read.  That applies to all
the other callers touched by this patch.

Thanks.