On Sat, Apr 22, 2023 at 07:12:13AM -0400, Jeff King wrote: > 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@xxxxxxxxxxxxxxxxxxxxxxx/ > 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