[PATCH v2] string.c: remove use of strcat() after strlen()

[PATCH v2] string.c: remove use of strcat() after strlen()

[James Tirta Halim]

Changes in v2:
return the realloc'd string

---
 Src/string.c | 8 ++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Src/string.c b/Src/string.c
index 5f439926e..99e31d6c3 100644
--- a/Src/string.c
+++ b/Src/string.c
@@ -28,6 +28,8 @@
 
 #include "zsh.mdh"
 
+#define MEMPCPY(dst, src, n) ((char *)memcpy(dst, src, n) + n)
+
 /**/
 mod_export char *
 dupstring(const char *s)
@@ -200,7 +202,10 @@ ztrduppfx(const char *s, int len)
 mod_export char *
 appstr(char *base, char const *append)
 {
-    return strcat(realloc(base, strlen(base) + strlen(append) + 1), append);
+    const size_t base_len = strlen(base);
+    const size_t append_len = strlen(append);
+    base = realloc(base, base_len + append_len + 1);
+    *(char *)MEMPCPY(base + base_len, append, append_len) = '\0';
+    return base;
 }
 
 /* Return a pointer to the last character of a string,
-- 
2.43.0

Re: [PATCH v2] string.c: remove use of strcat() after strlen()

[Bart Schaefer]

I take it this is intended to be an optimization?

Please do provide some background / rationale when sending a patch.  Thanks.

Re: [PATCH v2] string.c: remove use of strcat() after strlen()

[James]

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

Since we are already calling strlen() beforehand to know the size of
allocation, we can just memcpy() from BASE + BASE_LEN and avoid another
strlen() call in strcat(). We should prefer memcpy() because we always know
the length and most libc implementations provide an assembly implementation
of memcpy but maybe not strcpy(). Even if it is implemented in assembly,
memcpy() is likely to be faster than strcpy() since as the loop condition
strcpy() needs to check for zeros in SRC, whereas memcpy() can just
decrement the size.

I'm using a MEMPCPY macro because I don't know how zsh handles using
GNU/POSIX extensions.

On Sun, Dec 31, 2023 at 11:22 AM Bart Schaefer <schaefer@brasslantern.com>
wrote:

> I take it this is intended to be an optimization?
>
> Please do provide some background / rationale when sending a patch.
> Thanks.
>

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

Re: [PATCH v2] string.c: remove use of strcat() after strlen()

[Bart Schaefer]

On Sat, Dec 30, 2023 at 9:10 PM James <tirtajames45@gmail.com> wrote:
>
> Since we are already calling strlen() beforehand to know the size of allocation, we can just memcpy()

Fair enough, but the function you've optimized is called so seldom
that this is not going to have a meaningful effect on performance.

> I'm using a MEMPCPY macro because I don't know how zsh handles using GNU/POSIX extensions.

Please explain why that matters?  The macro is used exactly once after
being declared and there are no preprocesor conditionals around it.

Re: [PATCH v2] string.c: remove use of strcat() after strlen()

[Oliver Kiddle]

On 31 Dec, James wrote:
> Since we are already calling strlen() beforehand to know the size of
> allocation, we can just memcpy() from BASE + BASE_LEN and avoid another strlen
> () call in strcat(). We should prefer memcpy() because we always know the
> length and most libc implementations provide an assembly implementation of
> memcpy but maybe not strcpy(). Even if it is implemented in assembly, memcpy()
> is likely to be faster than strcpy() since as the loop condition strcpy() needs
> to check for zeros in SRC, whereas memcpy() can just decrement the size.
>
> I'm using a MEMPCPY macro because I don't know how zsh handles using GNU/POSIX
> extensions.

Many OS-specific extensions are used. We'd normally add a test in
configure.ac and then check for that so the macro could be wrapped in
#ifndef HAVE_MEMPCPY
And such macros may preserve the original lowercase which can be less
ugly. Calling the actual system mempcpy may be more efficient than the
macro because it probably already has a pointer to the end. FreeBSD
also has mempcpy().

If it is meant as an optimisation it could be worth verifying that it
has an effect. I don't especially find the modified code any easier to
read. I find it quicker and easier to scan when simple nested calls like
strlen() are not first taken out and assigned to a variable. If it does
make things faster, there may be other places in the code where appstr()
could have been used but strcat/realloc were used directly.

Oliver