From: Jeff King <peff@peff.net>
Date: Sun, 23 Apr 2023 00:35:53 GMT
Subject: Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`
Message-ID: <20230423003553.GA3953216@coredump.intra.peff.net>
In-Reply-To: <8d228041-d473-7f3f-87d7-543b350aab0e@web.de>

On Sat, Apr 22, 2023 at 05:53:05PM +0200, René Scharfe wrote:

> > Obviously one solution is to add the "runs" option to all variants. But
> > I'd be hesitant to burden existing callers. So I'd propose one of:
> >
> >   1. Make your _1() function public, with a name like _with_options() or
> >      something (though the function name is sadly already quite long).
> >      Leave string_list_split_in_place() as a wrapper that behaves as
> >      now, and have the few new callers use the with_options() variant.
> >
> >   2. Don't bother implementing the "runs" version. The only users would
> >      be test programs, and nobody cares much either way for their cases.
> >      Document in the commit message (and possibly above the function)
> >      that this isn't a strict replacement for strtok(). That would
> >      hopefully be enough for a potential caller to think about the
> >      behavior, and we can add "runs" if and when somebody actually wants
> >      it.
> 
> You can call string_list_remove_empty_items() to get rid of empty
> strings.  And if the single-char version calls a multi-char version
> under the hood then its only advantage is backward compatibility -- but
> converting all callers isn't that hard.  So the only string_list split
> function version you really need accepts multiple delimiters and
> preserves empty items and can be called string_list_split_in_place().

Ooh, I like that. I didn't think of processing after the fact. And
indeed, we have several spots already that do the split/remove_empty
pair.

And yes, it looks like there are only 7 callers which would need a
trivial update if we just switched to the multi-char version.

That's very compelling.

-Peff