Re: [PATCH 1/4] Add a new function, string_list_split_in_place()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2012, 05:47 UTC
Message-ID: <7vhar6pgxs.fsf@alter.siamese.dyndns.org>
In-Reply-To: <504D7082.9020903@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 13 quoted lines

> ...  Consider something like
>
>     struct string_list *split_file_into_words(FILE *f)
>     {
>         char buf[1024];
>         struct string_list *list = new string list;
>         list->strdup_strings = 1;
>         while (not EOF) {
>             read_line_into_buf();
>             string_list_split_in_place(list, buf, ' ', -1);
>         }
>         return list;
>     }

That is a prime example to argue that string_list_split() would make more sense, no? The caller does _not_ mind if the function mucks with buf, but the resulting list is not allowed to point into buf.

In such a case, the caller shouldn't have to _care_ if it wants to allow buf to be mucked with; it is already asking that the resulting list _not_ point into buf by setting strdup_strings (by the way, that is part of the function input, so think of it like various *opt variables passed into functions to tweak their behaviour). If the implementation can do so without sacrificing performance (and in this case, as you said, it can), it should take "const char *buf".

The above caller shouldn't have to choose between sl_split() and sl_split_in_place(), in other words.

So it appears to me that sl_split_in_place(), if implemented, should be kept as a special case for performance-minded callers that have full control of the lifetime rules of the variables they use, can set strdup_strings to false, and can let buf modified in place, and can accept list that point into buf.

Show 16 quoted lines

>>> + * Examples:
>>> + *   string_list_split_in_place(l, "foo:bar:baz", ':', -1) -> ["foo", "bar", "baz"]
>>> + *   string_list_split_in_place(l, "foo:bar:baz", ':', 1) -> ["foo", "bar:baz"]
>>> + *   string_list_split_in_place(l, "foo:bar:", ':', -1) -> ["foo", "bar", ""]
>> 
>> I would find it more natural to see a sentinel value against
>> "positive" to be 0, not -1.  "-1" gives an impression as if "-2"
>> might do something different from "-1", but Zero is a lot more
>> special.
>
> You have raised a good point and I think there is a flaw in the API, but
> I'm not sure I agree with you what the flaw is...
>
> The "maxsplit" argument limits the number of times the string should be
> split.  I.e., if maxsplit is set, then the output will have at most
> (maxsplit + 1) strings.

So "do not split, just give me the whole thing" would be maxsplit == 0 to split into (maxsplit+1) == 1 string. I think we are in agreement that your "-1" does not make any sense, and your documentation that said "positive" is the saner thing to do, no?