GNU bug report logs - #33144
Replace some loops with str functions

Previous Next

To reply to this bug, email your comments to 33144 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Kapus, Timotej" <timotej.kapus13 <at> imperial.ac.uk>
To: "bug-diffutils <at> gnu.org" <bug-diffutils <at> gnu.org>
Cc: "Cadar, Cristian" <c.cadar <at> imperial.ac.uk>
Subject: Replace some loops with str functions
Date: Wed, 24 Oct 2018 23:06:41 +0000

[Message part 1 (text/plain, inline)]

Hi,

I'm writing a program analysis that tries to find and replace some loops with str* functions. I'm trying to see if these replacements are a useful refactoring or the loops are intentional. I noticed that diff has a couple of these loops and wrote a patch (pasted below) that changes this.

I find strspn easier to read, communicate intention and has less LOC, but would be interested to hear why you would like to keep the loops to.

Cheers,
Timotej

From 38e7030f127f567fd2d02f27663168dde4472d57 Mon Sep 17 00:00:00 2001
From: Timotej Kapus <tk1713 <at> ic.ac.uk>
Date: Wed, 24 Oct 2018 23:47:22 +0100
Subject: [PATCH] maint: change 3 loops to strspn calls

  * src/ifdef.c: Change 3 loops to equivalent calls to strspn
---
 src/ifdef.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/ifdef.c b/src/ifdef.c
index 0ecc2c0..8046ac5 100644
--- a/src/ifdef.c
+++ b/src/ifdef.c
@@ -313,13 +313,10 @@ do_printf_spec (FILE *out, char const *spec,
   /* Scan printf-style SPEC of the form %[-'0]*[0-9]*(.[0-9]*)?[cdoxX].  */
   /* assert (*f == '%'); */
   f++;
-  while ((c = *f++) == '-' || c == '\'' || c == '0')
-    continue;
-  while (ISDIGIT (c))
-    c = *f++;
-  if (c == '.')
-    while (ISDIGIT (c = *f++))
-      continue;
+  f += strspn(f, "-\'0") + 1;
+  f += strspn(f, "0123456789") + 1;
+  if ((c = *f++) == '.')
+    f += strspn(f, "0123456789") + 1;
   c1 = *f++;

   switch (c)
--
2.7.4

[Message part 2 (text/html, inline)]

Message #8 received at 33144 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: "Kapus, Timotej" <timotej.kapus13 <at> imperial.ac.uk>, 33144 <at> debbugs.gnu.org
Cc: "Cadar, Cristian" <c.cadar <at> imperial.ac.uk>
Subject: Re: [bug-diffutils] bug#33144: Replace some loops with str functions
Date: Thu, 25 Oct 2018 12:57:10 -0700

On 10/24/18 4:06 PM, Kapus, Timotej wrote:
> I find strspn easier to read, communicate intention and has less LOC

On the other hand, it's probably slower for this particular use case, 
and the replacement code you wrote is not equivalent to the code it 
replaced because 'c' ends up having a different value.

Message #11 received at 33144 <at> debbugs.gnu.org (full text, mbox):

From: "Kapus, Timotej" <timotej.kapus13 <at> imperial.ac.uk>
To: Paul Eggert <eggert <at> cs.ucla.edu>, "33144 <at> debbugs.gnu.org"
 <33144 <at> debbugs.gnu.org>
Cc: "Cadar, Cristian" <c.cadar <at> imperial.ac.uk>
Subject: Re: [bug-diffutils] bug#33144: Replace some loops with str functions
Date: Fri, 26 Oct 2018 09:51:45 +0000

[Message part 1 (text/plain, inline)]

Thanks for your reply and apologies for sending a buggy patch. Yes I agree it's probably slower, but I doubt it has significant impact on the whole runtime. The insignifcant slowdown might be worth it, if you would find the code much clearer. In any case, I'm attaching an updated version of the patch, where I  proved c and c1 are eqvivalent at the end, up to the bound of 5 bytes, so should be fine this time.


From 4911d9676f1914fb4691061887396f2b21a0d036 Mon Sep 17 00:00:00 2001
From: Timotej Kapus <tk1713 <at> ic.ac.uk>
Date: Wed, 24 Oct 2018 23:47:22 +0100
Subject: [PATCH] maint: change 3 loops to strspn calls

  * src/ifdef.c: Change 3 loops to equivalent calls to strspn
---
 src/ifdef.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/ifdef.c b/src/ifdef.c
index 0ecc2c0..4533e3a 100644
--- a/src/ifdef.c
+++ b/src/ifdef.c
@@ -313,13 +313,13 @@ do_printf_spec (FILE *out, char const *spec,
   /* Scan printf-style SPEC of the form %[-'0]*[0-9]*(.[0-9]*)?[cdoxX].  */
   /* assert (*f == '%'); */
   f++;
-  while ((c = *f++) == '-' || c == '\'' || c == '0')
-    continue;
-  while (ISDIGIT (c))
-    c = *f++;
-  if (c == '.')
-    while (ISDIGIT (c = *f++))
-      continue;
+  f += strspn(f, "-\'0") + 1;
+  if(ISDIGIT(c = *(f - 1)))
+    f += strspn(f, "0123456789") + 1;
+  if ((c = *(f - 1)) == '.') {
+    f += strspn(f, "0123456789") + 1;
+    c = *(f - 1);
+  }
   c1 = *f++;

   switch (c)
--
2.7.4



________________________________
Od: Paul Eggert <eggert <at> cs.ucla.edu>
Poslano: četrtek, 25. oktober 2018 20:57:10
Za: Kapus, Timotej; 33144 <at> debbugs.gnu.org
Kp: Cadar, Cristian
Zadeva: Re: [bug-diffutils] bug#33144: Replace some loops with str functions

On 10/24/18 4:06 PM, Kapus, Timotej wrote:
> I find strspn easier to read, communicate intention and has less LOC

On the other hand, it's probably slower for this particular use case,
and the replacement code you wrote is not equivalent to the code it
replaced because 'c' ends up having a different value.

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.