Hi Elijah, On Thu, 25 Jul 2019, Elijah Newren wrote: > On Thu, Jul 25, 2019 at 1:12 PM Johannes Schindelin > <Johannes.Schindelin@xxxxxx> wrote: > > > > 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. Fair enough. I'm curious: what merge strategy are you planning on implementing? Ciao, Dscho
