Re: [PATCH v2 4/7] string-list: optionally trim string pieces split by string_list_split*()
- From
shejialuo <shejialuo@gmail.com>
- Date
- Aug 2, 2025, 01:51 UTC
- Message-ID
- <aI1vEzuD2DeiFSzW@mbp>
- In-Reply-To
- <xmqqa54iljw1.fsf@gitster.g>
On Fri, Aug 01, 2025 at 04:09:02PM -0700, Junio C Hamano wrote:
Show 27 quoted lines
> 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 ;-) >
Yes, I agree. Thanks for the wonderful explanation.
Show 15 quoted lines
> > 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. >
I get your point. In the later review, I think I should avoid commenting things using a subjetive idea. Really thanks for your suggestion.
Thanks,