Re: [PATCH v2 1/6] string-list: introduce `string_list_split_in_place_multi()`t

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 23, 2023, 02:38 UTC
Message-ID: <ZESaFXMcP6mhtBOv@nand.local>
In-Reply-To: <20230422111213.GB2969939@coredump.intra.peff.net>

On Sat, Apr 22, 2023 at 07:12:13AM -0400, Jeff King wrote:

Show 18 quoted lines

> On Tue, Apr 18, 2023 at 03:18:43PM -0400, Taylor Blau wrote:
>
> > Introduce a variant of the `string_list_split_in_place()` function that
> > takes a string of accepted delimiters.
> >
> > By contrast to its cousin `string_list_split_in_place()` which splits
> > the given string at every instance of the single character `delim`, the
> > `_multi` variant splits the given string any any character appearing in
> > the string `delim`.
> >
> > Like `strtok()`, the `_multi` variant skips past sequential delimiting
> > characters. For example:
> >
> >     string_list_split_in_place(&xs, xstrdup("foo::bar::baz"), ":", -1);
> >
> > would place in `xs` the elements "foo", "bar", and "baz".
>
> I have mixed feelings on this.

Hmm. I implemented it this way partially after reading your suggestion in [1], but also to allow using `string_list_split_in_place()` as a strict replacement for strtok().

And I agree that for the existing users of strtok(), which are all in the test helpers, this probably doesn't matter much either way. Though I feel like since we are banning strtok() over the whole tree, that we should provide a suitable replacement at the time we ban strtok(), not at the time somebody needs to rely on its non-empty fields behavior.

[1]: https://lore.kernel.org/git/20230418102320.GB508219@coredump.intra.peff.net/

Show 7 quoted lines

> 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.

I think that in general I'd prefer (2) to avoid polluting the list of declarations in string-list.h, but in this case I think that in this case it is the right thing to do.

Thanks, Taylor