Re: [PATCH 15/19] merge-recursive: split internal fields into a separate struct

From: Elijah Newren <newren@gmail.com>
Date: Jul 25, 2019, 20:27 UTC
Message-ID: <CABPp-BGpCEZ-40_SsT_vU+hCP=MiKzhky6yBr6G=UMnwLNR2Ow@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1907252206180.21907@tvgsbejvaqbjf.bet>

On Thu, Jul 25, 2019 at 1:12 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 16 quoted lines

>
> Hi Elijah,
>
> On Thu, 25 Jul 2019, Elijah Newren wrote:
>
> > merge_options has several internal fields that should not be set or read
> > by external callers.  This just complicates the API.  Move them into an
> > opaque merge_options_internal struct that is defined only in
> > merge-recursive.c and keep these out of merge-recursive.h.
>
> This method requires an extra `malloc()`/`free()` pair, leaving room for
> leaks.
>
> I'd rather keep the definition of that struct in `merge-recursive.h`,
> and make the `priv` field a full struct instead of a pointer. That would
> save on that extra allocation.

Yes, it has one extra malloc()/free() pair for the entire merge operation.  But:
  * That's a drop in the bucket compared to the allocations already
performed during a merge
  * The allocation and free happen in merge_start() and
merge_finalize().  Not much to keep track of, so not much room for
leaks.
  * I want what's currently in merge-recursive.h to be as simple as
possible; I implemented your suggestion first, but it doesn't simplify
as much as possible.

But, more importantly:
  * I want to write an alternative merge strategy providing drop-in
replacement functions for merge_trees(), merge_recursive(), and
merge_recursive_generic(). Defining merge_options_internal inside
merge-recursive.h would mean that I have to have _exactly_ the same
internal options in my implementation of merge-ort.c.  That doesn't
make sense.

Elijah