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.fat...@pengutronix.de>
> >> ---
> >> Originally posted at:
> >> https://lore.barebox.org/barebox/20221027073334.gs6...@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.fat...@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 |
