threads / patch / 36777

patchImprove function dir.c:trim_trailing_spaces()

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

## tl;dr

5 messages between May 28, 2014 and May 30, 2014. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Pasha Bolokhov· May 28, 2014, 23:45 UTC · lore

Move backwards from the end of the string (more efficient for lines which do not have trailing spaces or have just a couple). Slightly more rare occurrences of 'text \ ' with a backslash in between spaces are handled correctly. Namely, the code in 8ba87adad6 does not reset 'last_space' when a backslash is encountered and the above line stays intact as a result --- How about trailing tabs?

 dir.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Show changes to dir.c +11 −14

diff --git a/dir.c b/dir.c
index eb6f581..3315eea 100644
--- a/dir.c
+++ b/dir.c
@@ -508,21 +508,18 @@ void clear_exclude_list(struct exclude_list *el)
 
 static void trim_trailing_spaces(char *buf)
 {
-	int i, last_space = -1, nr_spaces, len = strlen(buf);
-	for (i = 0; i < len; i++)
-		if (buf[i] == '\\')
-			i++;
-		else if (buf[i] == ' ') {
-			if (last_space == -1) {
-				last_space = i;
-				nr_spaces = 1;
-			} else
-				nr_spaces++;
-		} else
-			last_space = -1;
-
-	if (last_space != -1 && last_space + nr_spaces == len)
+	int i, last_space, bslash = 0, len = strlen(buf);
+
+	if (len == 0 || buf[len - 1] != ' ')
+		return;
+	for (i = len - 2; i >= 0 && buf[i] == ' '; i--) ;
+	last_space = i + 1;
+	for ( ; i >=0 && buf[i] == '\\'; i--) bslash ^= 1;
+
+	if (!bslash)
 		buf[last_space] = '\0';
+	else if (bslash && last_space < len - 1)
+		buf[last_space + 1] = '\0';
 }
 
 int add_excludes_from_file_to_list(const char *fname,

-- 
1.9.1

Jeff King· May 29, 2014, 20:13 UTC · re: Pasha Bolokhov · lore

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

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).

> 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

Pasha Bolokhov· May 29, 2014, 21:34 UTC · re: Jeff King · lore

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

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

Jeff King· May 30, 2014, 02:04 UTC · re: Pasha Bolokhov · lore

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

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

Show 7 quoted lines

> > 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".

Show 7 quoted lines

>      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.

Show 10 quoted lines

>     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

Junio C Hamano· May 30, 2014, 18:45 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> 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".

Sensible.

> 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.

FWIW, I think I agree.