0

Does this implementation make any sense to you? I'm trying to write a function that will concatenate two strings and can be called as appendstr(&dest, "xyz"); I'm not sure at all if is a good practice what I'm doing here, reallocating the space for newptr over origptr and then free it and make it equal to newptr.

void *appendstr(char **origptr, const char *strdata)
{
    size_t len1 = strlen(*origptr);
    size_t len2 = strlen(strdata);
    char *newptr = realloc(*origptr, len1 + len2 + 1);
    if (newptr != NULL)
    {
        memcpy(newptr + len1, strdata, len2 + 1);
        free(*origptr);
        *origptr = newptr;
    }
    return newptr;
}

All I'm trying to do is to not change anything in *origptr until I'm sure that there is no problem with the memory allocation and only then, do the concatenation. Also, another concern is if I'm allocating exactly the amount of memory that I need.

Hariprasad
  • 3,556
  • 2
  • 24
  • 40
Zack
  • 385
  • 2
  • 3
  • 21
  • 5
    Don't need to free(*origptr). realloc() does that if newptr is different from origptr. – mcleod_ideafix Dec 21 '13 at 22:37
  • 1
    Is there a reason you don't just use strcat(...)? [See this post.][1] [1]: http://stackoverflow.com/questions/308695/c-string-concatenation – FuzzyBunnySlippers Dec 21 '13 at 22:45
  • @mcleod_ideafix does realloc free origptr even if the memory allocation fails? – Zack Dec 21 '13 at 22:59
  • 3
    If `realloc()` cannot allocate the new space, it returns NULL and the original pointer is still valid, but with no extra space. – mcleod_ideafix Dec 21 '13 at 23:03
  • 2
    @NonlinearIdeas: Yes, there are good reasons not to use `strcat()`, notably that it has to rescan the first `len1` bytes of the reallocated space to find the null that the code already knows is at `newptr + len1`. Using `memcpy()` saves that rescan. – Jonathan Leffler Dec 21 '13 at 23:12
  • 1
    Tip: if you intend to concatenate many times, it's probably better to always round to the next power of two, to make the approach to the final size O(log N). – Matteo Italia Dec 21 '13 at 23:27
  • See also [Allocating memory for two concatenated strings](http://stackoverflow.com/questions/20720459/allocating-memory-for-two-concatenated-strings). – Jonathan Leffler Dec 22 '13 at 03:56

1 Answers1

4

This should be enough. It's just your code without the free()

void *appendstr(char **origptr, const char *strdata)
{
    size_t len1 = strlen(*origptr);
    size_t len2 = strlen(strdata);
    char *newptr = realloc(*origptr, len1 + len2 + 1);
    if (newptr != NULL)
    {
        memcpy(newptr + len1, strdata, len2 + 1);
        *origptr = newptr;
    }
    return newptr;
}

Returns NULL if it couldn't concatenate the strings, and *origptr is not changed. Otherwise, it returns the (possibly new) allocated pointer with data concatenated. *origptr then will have the same value as the returned value.

memcpy() instead of strcat() because strcat() needs to know where the string ends by calling strlen() internally (or doing its own version of strlen() ). As anyway you call strlen() because you need the length of the string to calculate the amount of memory to allocate, you can use memcpy() to directly copy the second string right after the first one, skipping an implicit second call to strlen() if were using strcat().

mcleod_ideafix
  • 11,128
  • 2
  • 24
  • 32
  • Thanks for your answer! Should I use memmove instead of memcpy? – Zack Dec 21 '13 at 23:19
  • 1
    About `memcpy()` vs `memmove()`, read this: http://stackoverflow.com/questions/4415910/memcpy-vs-memmove – mcleod_ideafix Dec 21 '13 at 23:23
  • Is there any way that I can get a memory overrun with this implementation, maybe by getting a second parameter that is not null terminated? If it is posible, how could I fix this? – Zack Dec 21 '13 at 23:40
  • 1
    I think that would be a topic for another question ;) Of course you will get into serious problems if the second string is not null terminated. For start, the strlen() call may segfault. – mcleod_ideafix Dec 22 '13 at 00:00