Re: [PATCH v2 4/7] string-list: optionally trim string pieces split by string_list_split*()

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

shejialuo <shejialuo@gmail.com> writes:

> I agree with you that we would introduce another variable. However, the
> thing I quite dislike is that we do ltrim inside the current function
> and we do rtrim inside `append_one`.

Your "quite dislike" does not matter unless backed by a reason why
it is not good, and for that, you need to think a bit deeper.  Then
you will hopefully appreciate why the current arrangement is more
optimal ;-)

There is a clear separation of tasks between this caller-callee
pair.  The caller is responsible for finding where the current token
ends, and the callee is responsible for massaging the current token
into the resulting list.

But ltrim needs to be done in the caller for this to be efficient.

Imagine the case where you want to allow both non-empty and trim
behaviour at the same time, and use a whitespace character as
delimiter.  If your token has leading whitespaces, instead of
chopping them into zero-length ranges and feeding it to append_one()
one by one, only to have them discarded (due to non-empty being
set), ltrimming in the caller before it decides where the next token
(i.e. "end") starts is far more efficient.  It may be more
conceptually cleaner, but cleanliness is more subjective ;-)

> My thinking is that we should handle the [p, end) string in the same
> place.

Again this sounds no more than a subjective "quite dislike".  Is
there a reason why anybody would want to insist these two things be
done at the same location?

You could satisfy the subjective "same place requirement" by
inlining the helper function into its sole caller and still keep the
current arrangement to ltrim before finding "end", of course.  But
at that point, I would have to say that it is a tail wagging the
dog.  You are making the code worse by destroying the caller-callee
division of responsibilities, only to satisfy a subjetive "quite
dislike" criteria.