# [PATCH] Improve function dir.c:trim_trailing_spaces() 5 messages from 2014-05-28 to 2014-05-30. Participants: Pasha Bolokhov, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/36777 ## Pasha Bolokhov, 2014-05-28 23:45 Subject: [PATCH] Improve function dir.c:trim_trailing_spaces() Message-ID: <1401320757-9360-1-git-send-email-pasha.bolokhov@gmail.com> URL: https://gitlist.dev/e/1401320757-9360-1-git-send-email-pasha.bolokhov%40gmail.com ``` 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(-) 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, 2014-05-29 20:13 Subject: Re: [PATCH] Improve function dir.c:trim_trailing_spaces() Message-ID: <20140529201334.GA17355@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140529201334.GA17355%40sigill.intra.peff.net In-Reply-To: <1401320757-9360-1-git-send-email-pasha.bolokhov@gmail.com> ``` 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, 2014-05-29 21:34 Subject: Re: [PATCH] Improve function dir.c:trim_trailing_spaces() Message-ID: URL: https://gitlist.dev/e/CAKpPgveDfNq%2BkxWCukULPqvn7H2XsfS8maC0ExzvHzFGiEAPbg%40mail.gmail.com In-Reply-To: <20140529201334.GA17355@sigill.intra.peff.net> ``` On Thu, May 29, 2014 at 1:13 PM, Jeff King wrote: > 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? > >> 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, 2014-05-30 02:04 Subject: Re: [PATCH] Improve function dir.c:trim_trailing_spaces() Message-ID: <20140530020444.GH28683@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140530020444.GH28683%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2014-05-30 18:45 Subject: Re: [PATCH] Improve function dir.c:trim_trailing_spaces() Message-ID: URL: https://gitlist.dev/e/xmqqa99z9kbz.fsf%40gitster.dls.corp.google.com In-Reply-To: <20140530020444.GH28683@sigill.intra.peff.net> ``` Jeff King writes: > 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. ```