On Thu, May 29, 2014 at 02:34:33PM -0700, Pasha Bolokhov wrote: > > However, I doubt it makes that much of a difference in practice, so > > unless it's measurable, I would certainly go with the version that is > > more readable (and correct, of course). > > Sorry, just to recap, you would go with the existing version > (which needs correction), or with the one that is being suggested? (I > agree I can format the style a tiny bit better in the latter one) I actually think the original left-to-right is a little easier to follow, but I do not feel strongly. I mainly meant "argue based on readability and correctness, do not argue based on speed". > Tests should not be a big problem, although it's kind of clumsy > to test an internal function which does not really give any output > (you can only measure the outcome). Just again to stress, I have > tested both implementation extensively and the suggested new > implementation gives the correct answers for all your examples below > and all others. If I show this with explicit "t/" tests, will it > suffice then? Yes. I think specifically that you can extend the tests at the end of t0008. > Basically what I suggest is > > -- either: improve the existing function such that it does correctly > that "text \ " case, and also does not use 'strlen' since it anyway > moves left to right > > -- or: use the new suggested implementation (and just brush the > formatting a bit), and perhaps borrow 'len' from the calling routine > > And add tests in any case. What is the preference? I'd be OK with either, though I have a slight preference for the first, just because I find the "bslash ^= 1" bit of yours, while clever, a bit hard to follow. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html