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@xxxxxxxxxxxxxx> > >> --- > >> Originally posted at: https://lore.barebox.org/barebox/20221027073334.GS6702@xxxxxxxxxxxxxx/ > > > > 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@xxxxxxxxxxxxxx> > >> --- > >> 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 |
