Re: [PATCH 1/5] xdiff: support external hunks via xpparam_t

>
> Michael Montalbo <mmontalbo@gmail.com> writes:
>
> >> > +      * Clear changed[] arrays.  xdl_prepare_env() may have dirtied
> >> > +      * them via xdl_cleanup_records().  The allocation is nrec + 2
> >> > +      * elements; changed points one past the start (see xprepare.c).
> >> > +      */
> >> > +     memset(xe->xdf1.changed - 1, 0,
> >> > +            (xe->xdf1.nrec + 2) * sizeof(bool));
> >> > +     memset(xe->xdf2.changed - 1, 0,
> >> > +            (xe->xdf2.nrec + 2) * sizeof(bool));
> >>
> >> This, especially the starting offset of -1, looks horrible.  The
> >> internal layout of xdfenv_t might happen to match the way the above
> >> code expects, which is how xdl_prepare_ctx() may have give you, but
> >> it somehow feels brittle.  I guess the assumption that changed[]
> >> does not point at the beginning of the allocated area (e.g., it is a
> >> no-no to free(xe->xdf1.changed) or realloc() it) is so pervasive that
> >> it cannot be helped.  Sigh.
> >>
> >
> > Agreed it is ugly. I wanted to make sure the entire changed[] including
> > sentinels were clear as a defensive measure for downstream callers
> > (xdl_change_compact). I agree this results in something that is ugly
> > and brittle, but in the end I thought it was superior to relying on the
> > fact that upstream zeroes the entire changed[] array. Maybe if the
> > comment was more explicit about why this is happening it would be
> > helpful?
>
> Perhaps make these memset() into calls to a helper function that is
> defined in xdiff/xprepare.c with a descriptive name and placed near
> where xdl_prepare_ctx() is.  That way, the patch in question does
> not even have to expose the strangeness of changed[] (i.e., it has 2
> more elements than it would normally contain to make the memory
> region for changed[-1] and changed[N] valid, and freeing it requires
> free(changed-1)) to the code path.  It only needs to say "Hey, I am
> clearing changed[] arrays because of XXX" without having to say "by
> the way, the memory layout of changed[] is strange this way", the
> latter of which is not exactly of interest for readers of this code.
>
> >     /*
> >      * Clear changed[] arrays including sentinels.
> >      * xdl_prepare_env() may have dirtied them via
> >      * xdl_cleanup_records(), and xdl_change_compact() reads
> >      * the sentinel at changed[-1] during backward scans.
> >      */
>
> And this belongs in xdiff/xprepare.c near that new helper function.