Re: [PATCH v2 2/7] string-list: align string_list_split() with its _in_place() counterpart

[Junio C Hamano <gitster@pobox.com>]

shejialuo <shejialuo@gmail.com> writes:

> Sorry to make you confused. Because there are some other changes where
> you don't use `static`. That's the main reason why I ask this question.

Hmph, are there new things that are not static but have good reasons
to be?  I am not aware of them offhand.

> --- a/t/helper/test-path-utils.c
> +++ b/t/helper/test-path-utils.c
> @@ -348,6 +348,7 @@ int cmd__path_utils(int argc, const char **argv)
>  	if (argc == 4 && !strcmp(argv[1], "longest_ancestor_length")) {
>  		int len;
>  		struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
> +		const char path_sep[] = { PATH_SEP, '\0' };
>  		char *path = xstrdup(argv[2]);
>
>> > And I have a design question: by using "PATH_SEP", we need to convert
>> > this character to be string. Should we create a new variable named
>> > "PATH_SEP_STR" or whatever to do that?
>> 
>> Sorry, but I do not understand the question.  You want to see
>> something like
>> 
>> 	#define PATH_SEP_STR "/"
>> 
>> you mean?  I do not offhand see why anybody would want to do so.
>> 
>
> Yes, that's my question. Because I see that we would define `path_sep`
> array in many place, so I wonder whether we could use such macro.

Why would we define path_sep[] in many places?  There is only one
here, and no existing code outside this helper needs one.

If it becomes necessary, I suspect that a global variable, i.e.

    === in some header file, perhaps git-compat-util.h ===
    extern const char PATH_SEP_STR[];

    === in a C source file, perhaps dir.c ===
    const char PATH_SEP_STR[] = { PATH_SEP, '\0' };

would be more efficient than having such #define and use site across
the codebase.

Using PATH_SEP_STR defined as a literal string "/" and sprinkling
many copies of it in the source files everywhere gives compilers
opportunity to consolidate those that appear in each file into one
copy (as these are literals and constant, they can be shared), but
would not give opportunity to do so across compilation units so
easily.  With an explicit singleton global instance, that is
trivial.

If we end up using great many number of them to matter, that is.  As
I said above, we have no evidence that would be the case, and that
is why I didn't make such a change in the first place.

Also, if many code paths need to split a path into pieces, it is
much more likely for us to give them a single well-designed helper
function for doing exactly that, than to have PATH_SEP_STR[] that
can be used by them from everywhere and have them use it to split
their paths individually.