Re: [PATCH 04/17] xdiff: delete xdl_get_rec() in xemit
- From
Elijah Newren <newren@gmail.com>
- Date
- Sep 9, 2025, 08:56 UTC
- Message-ID
- <CABPp-BFNSj5rLYqYXCbqmg-kKwfV8BewjoCx+7j3vhT6waojFw@mail.gmail.com>
- In-Reply-To
- <ab17d8c23f0ca48374416ec1eb34128585795736.1757274320.git.gitgitgadget@gmail.com>
On Sun, Sep 7, 2025 at 12:45 PM Ezekiel Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 21 quoted lines
>
> 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.
>
> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> ---
> xdiff/xemit.c | 38 +++++++++++++-------------------------
> 1 file changed, 13 insertions(+), 25 deletions(-)
>
> diff --git a/xdiff/xemit.c b/xdiff/xemit.c
> index 1d40c9cb40..2161ac3cd0 100644
> --- a/xdiff/xemit.c
> +++ b/xdiff/xemit.c
> @@ -22,23 +22,13 @@
>
> #include "xinclude.h"
>
> -static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec) {
>Can we remove this line too, to simplify the diff? (i.e. make there only be one blank line between the include of xinclude.h and xdl_emit_record?
Show 14 quoted lines
> - *rec = xdf->recs[ri]->ptr;
> -
> - return xdf->recs[ri]->size;
> -}
> -
> -
> -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) {
> +static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *ecb)
> +{The change of the opening curly brace to be on a new line does match our general coding guidelines, but cleanups like this should be in a separate patch.
Show 5 quoted lines
> + xrecord_t *rec = xdf->recs[ri]; > > + if (xdl_emit_diffrec(rec->ptr, rec->size, pre, strlen(pre), ecb) < 0) > return -1; > - }
While in this case you were modifying the line in question and thus fixing the code to also not use curly braces around a single statement, which is more justified, it still makes the patch slightly harder for reviewers to read since you are doing multiple things (what you said in the commit message, plus cleaning up style "violations"). It'd be better to leave the existing style violations in place, or fix them in a separate patch.
Show 37 quoted lines
>
> return 0;
> }
> @@ -120,11 +110,11 @@ static long def_ff(const char *rec, long len, char *buf, long sz)
> static long match_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri,
> char *buf, long sz)
> {
> - const char *rec;
> - long len = xdl_get_rec(xdf, ri, &rec);
> + xrecord_t *rec = xdf->recs[ri];
> +
> if (!xecfg->find_func)
> - return def_ff(rec, len, buf, sz);
> - return xecfg->find_func(rec, len, buf, sz, xecfg->find_func_priv);
> + return def_ff(rec->ptr, rec->size, buf, sz);
> + return xecfg->find_func(rec->ptr, rec->size, buf, sz, xecfg->find_func_priv);
> }
>
> static int is_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri)
> @@ -160,14 +150,12 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,
>
> static int is_empty_rec(xdfile_t *xdf, long ri)
> {
> - const char *rec;
> - long len = xdl_get_rec(xdf, ri, &rec);
> + xrecord_t *rec = xdf->recs[ri];
> + long i = 0;
>
> - while (len > 0 && XDL_ISSPACE(*rec)) {
> - rec++;
> - len--;
> - }
> - return !len;
> + for (; i < rec->size && XDL_ISSPACE(rec->ptr[i]); i++);
> +
> + return i == rec->size;
> }I agree that the code is easier to follow without the aliasing.