[PATCH] simplify __procfdname by folding the 0 case

[PATCH] simplify __procfdname by folding the 0 case

[Fangrui Song]

---
 src/internal/procfdname.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/src/internal/procfdname.c b/src/internal/procfdname.c
index 697e0bdc..5046abaa 100644
--- a/src/internal/procfdname.c
+++ b/src/internal/procfdname.c
@@ -2,12 +2,7 @@ void __procfdname(char *buf, unsigned fd)
 {
 	unsigned i, j;
 	for (i=0; (buf[i] = "/proc/self/fd/"[i]); i++);
-	if (!fd) {
-		buf[i] = '0';
-		buf[i+1] = 0;
-		return;
-	}
-	for (j=fd; j; j/=10, i++);
-	buf[i] = 0;
-	for (; fd; fd/=10) buf[--i] = '0' + fd%10;
+	for (j=fd; i++, j /= 10; );
+	buf[i] = '\0';
+	while (buf[--i] = '0' + fd%10, fd /= 10);
 }
-- 
2.18.0

Re: [PATCH] simplify __procfdname by folding the 0 case

[Alexander Monakov]

In 2016 I sent a more comprehensive cleanup, please review the thread
starting at http://openwall.com/lists/musl/2016/02/21/2

Some notes on the patch below.

On Sun, 2 Sep 2018, Fangrui Song wrote:
> --- a/src/internal/procfdname.c
> +++ b/src/internal/procfdname.c
> @@ -2,12 +2,7 @@ void __procfdname(char *buf, unsigned fd)
>  {
>  	unsigned i, j;
>  	for (i=0; (buf[i] = "/proc/self/fd/"[i]); i++);
> -	if (!fd) {
> -		buf[i] = '0';
> -		buf[i+1] = 0;
> -		return;
> -	}
> -	for (j=fd; j; j/=10, i++);
> -	buf[i] = 0;
> -	for (; fd; fd/=10) buf[--i] = '0' + fd%10;
> +	for (j=fd; i++, j /= 10; );

This is not correct as it only increments i once. A do-while loop would do the
job better here.

> +	buf[i] = '\0';
> +	while (buf[--i] = '0' + fd%10, fd /= 10);

Likewise, a do-while loop would be more suitable here.

>  }

Alexander

Re: [PATCH] simplify __procfdname by folding the 0 case

[Fāng-ruì Sòng]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

On Sun, Sep 2, 2018 at 4:15 AM Alexander Monakov <amonakov@ispras.ru> wrote:

> In 2016 I sent a more comprehensive cleanup, please review the thread
> starting at http://openwall.com/lists/musl/2016/02/21/2
>
> Some notes on the patch below.
>
> On Sun, 2 Sep 2018, Fangrui Song wrote:
> > --- a/src/internal/procfdname.c
> > +++ b/src/internal/procfdname.c
> > @@ -2,12 +2,7 @@ void __procfdname(char *buf, unsigned fd)
> >  {
> >       unsigned i, j;
> >       for (i=0; (buf[i] = "/proc/self/fd/"[i]); i++);
> > -     if (!fd) {
> > -             buf[i] = '0';
> > -             buf[i+1] = 0;
> > -             return;
> > -     }
> > -     for (j=fd; j; j/=10, i++);
> > -     buf[i] = 0;
> > -     for (; fd; fd/=10) buf[--i] = '0' + fd%10;
> > +     for (j=fd; i++, j /= 10; );
>
> This is not correct as it only increments i once. A do-while loop would do
> the
> job better here.
>

May I defend for myself?  for (j=fd; i++, j /= 10; );
i++ is in the loop condition so it will be incremented multiple times.


> > +     buf[i] = '\0';
> > +     while (buf[--i] = '0' + fd%10, fd /= 10);
>
> Likewise, a do-while loop would be more suitable here.
>

I'm still learning musl's code style so may likely make the code look ugly
:)


> >  }
>
> Alexander
>

[-- Attachment #2: Type: text/html, Size: 2160 bytes --]

Re: [PATCH] simplify __procfdname by folding the 0 case

[Alexander Monakov]

[-- Attachment #1: Type: text/plain, Size: 446 bytes --]

On Sun, 9 Sep 2018, Fāng-ruì Sòng wrote:
> > > -     for (; fd; fd/=10) buf[--i] = '0' + fd%10;
> > > +     for (j=fd; i++, j /= 10; );
> >
> > This is not correct as it only increments i once. A do-while loop would do
> > the
> > job better here.
> >
> 
> May I defend for myself?  for (j=fd; i++, j /= 10; );
> i++ is in the loop condition so it will be incremented multiple times.

Sorry, my mistake there (I misread the change).

Alexander