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

From: Pasha Bolokhov <pasha.bolokhov@gmail.com>
Date: May 29, 2014, 21:34 UTC
Message-ID: <CAKpPgveDfNq+kxWCukULPqvn7H2XsfS8maC0ExzvHzFGiEAPbg@mail.gmail.com>
In-Reply-To: <20140529201334.GA17355@sigill.intra.peff.net>

On Thu, May 29, 2014 at 1:13 PM, Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

> On Wed, May 28, 2014 at 04:45:57PM -0700, Pasha Bolokhov wrote:
>
>> Move backwards from the end of the string (more efficient for
>> lines which do not have trailing spaces or have just a couple).
>
> The original code reads the string from left to right. In theory, that
> means we could get away with not calling strlen() at all, over a
> right-to-left that must start with a call to strlen().
>
> That being said, I think we already have the length in the calling
> function, so you could probably avoid the strlen() altogether, which
> makes a right-to-left function more efficient.
>
> 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)

     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?

    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?

Show 19 quoted lines

>
>> Slightly more rare occurrences of 'text  \    ' with a backslash
>> in between spaces are handled correctly.
>
> Can you add a test for this?
>
> Also, if you are refactoring this function, I think it makes sense to
> check that:
>
>   "foo\\ "
>
> and
>
>   "foo\\\ "
>
> match "foo\" and "foo\ ", respectively (I think they do with your patch,
> but it is a tricky case that is not immediately obvious).
>
> -Peff