4

I decided to try to make a concatenating function as strcat doesn't work for chars, only strings.

#include <stdio.h>
#include <string.h>

char concat(char a[], char b[]);

int main ()
{
   char *con =  concat("hel", "lo");
   return(0);
}

char concat(char a[], char b[]){
   int lena = strlen(a);
   int lenb = strlen(b);
   char con[lena+lenb];
   con[0] = a;
   con[lena] = b;
   printf("%s", con);
   return con;
}

This code prints "Ã…ÃÆ". Not sure where I am going wrong?

Thanks

Alfie
  • 1,903
  • 4
  • 21
  • 32
  • 2
    Did you try to compile it with all warnings enabled? – Sergio Oct 22 '16 at 18:43
  • 2
    Because you return a pointer to a local variable that ceases to exist when the function returns. Common problem. Good compilers warn about it. Is yours warning you? If so, pay heed. If not, find out how to make it give you important and useful warnings. If it can't give those warnings, get a better compiler. (You also don't allocate enough space — you didn't allow for null termination. And the copying mechanism you're using should be generating screeds of warnings too. You need to use `strcpy()` or `strcat()` or similar mechanisms to copy strings in C.) – Jonathan Leffler Oct 22 '16 at 18:44
  • 1
    And another funny thing: you say _"I decided to try to make a concatenating function as `strcat` doesn't work for chars, only strings"_ but your function works with strings, not chars. You need to reword that statement — or rewrite the code (but that's mostly necessary anyway). – Jonathan Leffler Oct 22 '16 at 18:48

1 Answers1

5

First, you shouldn't return reference to temporary

char con[lena+lenb];

(note that the garbage you get doesn't come from that since you print within the function)

Second, you don't allocate enough memory: should be (with first problem fixed):

char *con = malloc(lena+lenb+1);

then use strcpy/strcat anyway, it's faster, and your original code doesn't do anything useful (mixing chars with array of chars & the size of the arrays isn't known at this moment: that's the reason of the garbage you're getting):

strcpy(con,a);
strcat(con,b);

Or as some suggest that they're unsafe functions, and since we know the size of inputs we can write:

memcpy(con,a,lena);
memcpy(con+lena,b,lenb+1);

Also: the prototype of concat is really wrong. It should be:

 char *concat(const char *a, const char *b){

(as it returns a pointer on chars not a char. And the arguments should be constant pointers so you can use your function with any string)

and you're done (don't forget to free the string when you're done with it)

Fixed code (tested, surprisingly returns hello, maybe because it compiles without errors with gcc -Wall -Wwrite-strings -Werror. My advice: turn the warnings on and read them. You'll solve 80% of your problems that way):

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

char *concat(const char *a, const char *b);

int main ()
{
    char *con =  concat("hel", "lo");
    printf("%s\n",con);
    return(0);
}

char *concat(const char *a, const char *b){
    int lena = strlen(a);
    int lenb = strlen(b);
    char *con = malloc(lena+lenb+1);
    // copy & concat (including string termination)
    memcpy(con,a,lena);
    memcpy(con+lena,b,lenb+1);        
    return con;
}
Jean-François Fabre
  • 137,073
  • 23
  • 153
  • 219
  • Beware that `strcpy()` and `strcat()` are unsafe functions. – glauxosdever Oct 22 '16 at 18:54
  • 2
    @glauxosdever: Not in this context; the measurement has been done and the usage proposed here is entirely safe, Microsoft's compiler notwithstanding. – Jonathan Leffler Oct 22 '16 at 18:56
  • Thank you so much! What does `char *con = malloc(lena+lenb+1)` do differently to `char con[lena+lenb];` – Alfie Oct 22 '16 at 19:02
  • Right. In this context they are not unsafe, but encouraging use of such functions may lead to use of them even in unsafe manners. – glauxosdever Oct 22 '16 at 19:03
  • @glauxosdever: do you propose that we perform the copy char by char in a loop? would it be better? – Jean-François Fabre Oct 22 '16 at 19:05
  • @Alfie: first difference: there's 1 more byte for string termination! and then the memory is guaranteed not to be trashed when returning from the function. You should _really_ check the warnings, fix them don't ignore them. – Jean-François Fabre Oct 22 '16 at 19:06
  • I may be putting words into glauxosdever's mouth, but I suspect that they'd rather you used `strcpy_s()` and `strcat_s()` — which are defined and preferred by Microsoft, and defined in Annex K of C11 but not widely implemented except on Windows. Or maybe he has in mind using the `mem*()` functions. – Jonathan Leffler Oct 22 '16 at 19:07
  • You should use `strncat` or, even better, `strlcat` if available. I think `strcat_s()` would work too, although I have never used it. I guess someone should finally invent some function that dynamically allocates memory for concatenated strings – glauxosdever Oct 22 '16 at 19:09
  • 2
    @glauxosdever: what is the meaning of the third argument to `strncat()`? If you're about to say "the length of the destination string", you've just validated why `strncat()` is a disaster. Do not use `strncat()` — if you must use it, ensure the target has a null byte in the first byte, and then `target[0] = '\0'; strncat(target, source, sizeof(target));` is safe (and arguably better than `strncpy()`), but it is a specialized usage indeed. In the general case, that `sizeof(target)` is wrong! Horribly wrong! The `strl*()` functions are OK, but not as portable as all that. – Jonathan Leffler Oct 22 '16 at 19:15
  • in that particular case, maybe the only advantage of memcpy is that it doesn't test the null termination, so maybe faster? but not sure. I have edited my post to make everybody happy. – Jean-François Fabre Oct 22 '16 at 19:17
  • Indeed `strncat()` is only *slighly* better than `strcat()`, but it is still better. As I said, the ideal function for string concatenation would be one that dynamically allocates memory for concatenated strings. But I think this is really not the place to argue. – glauxosdever Oct 22 '16 at 19:21