Re: [PATCH 047/112] string: reduce strjoin runtime, drop trailing separator

[Sascha Hauer <sha@pengutronix.de>]

On Mon, Jan 08, 2024 at 08:18:46AM +0100, Ahmad Fatoum wrote:
> On 08.01.24 08:11, Sascha Hauer wrote:
> > On Wed, Jan 03, 2024 at 07:12:07PM +0100, Ahmad Fatoum wrote:
> >> The implementation of strjoin is a bit suboptimal. The destination
> >> string is traversed from the beginning due to strcat and we have a
> >> left-over separator at the end, while it should only be in-between.
> >>
> >> Fix this.
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> Originally posted at: https://lore.barebox.org/barebox/20221027073334.GS6702@pengutronix.de/
> > 
> > Once again I ended up reviewing a suboptimal version of strjoin() first
> > just to find my potential comments addressed in the next patch. So my
> > comment to the last version still stands: Please implement a good
> > version of strjoin() first and then switch over to use it.
> 
> The first patch just moves code around. I find it completely valid to move
> code before doing changes to it...

I find this valid as well, but you have not just moved it around, you
have introduced a new library function in the same step. As such it
should be in good shape when introducing it.

Sascha

> 
> > 
> >>
> >> Changes:
> >>   - remove if statemnt in loop (Sascha)
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  lib/string.c | 15 ++++++++++-----
> >>  1 file changed, 10 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/lib/string.c b/lib/string.c
> >> index d8e5edd40648..695e50bc8fc1 100644
> >> --- a/lib/string.c
> >> +++ b/lib/string.c
> >> @@ -1005,7 +1005,7 @@ char *strjoin(const char *separator, char **arr, size_t arrlen)
> >>  {
> >>  	size_t separatorlen;
> >>  	int len = 1; /* '\0' */
> >> -	char *buf;
> >> +	char *buf, *p;
> >>  	int i;
> >>  
> >>  	separatorlen = strlen(separator);
> >> @@ -1013,13 +1013,18 @@ char *strjoin(const char *separator, char **arr, size_t arrlen)
> >>  	for (i = 0; i < arrlen; i++)
> >>  		len += strlen(arr[i]) + separatorlen;
> > 
> > Not that it matters much for memory usage, but for consistency you could
> > drop the final separatorlen just like you did for copying the strings
> > below.
> > 
> > Sascha
> > 
> >>  
> >> -	buf = xzalloc(len);
> >> +	if (!arrlen)
> >> +		return xzalloc(1);
> >>  
> >> -	for (i = 0; i < arrlen; i++) {
> >> -		strcat(buf, arr[i]);
> >> -		strcat(buf, separator);
> >> +	p = buf = xmalloc(len);
> >> +
> >> +	for (i = 0; i < arrlen - 1; i++) {
> >> +		p = stpcpy(p, arr[i]);
> >> +		p = mempcpy(p, separator, separatorlen);
> >>  	}
> >>  
> >> +	stpcpy(p, arr[i]);
> >> +
> >>  	return buf;
> >>  }
> >>  EXPORT_SYMBOL(strjoin);
> >> -- 
> >> 2.39.2
> >>
> >>
> >>
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |