Re: [PATCH] Improve function dir.c:trim_trailing_spaces()

On Thu, May 29, 2014 at 02:34:33PM -0700, Pasha Bolokhov wrote:

quoted

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