views:

568

answers:

6

Hi,

Is it the correct way of allocating memory to a char*.

char* sides ="5";

char* tempSides;

tempSides = (char*)malloc(strlen(inSides) * sizeof(char));
+6  A: 

Almost. Strings are NULL terminated, so you probably want to allocate an extra byte to store the NULL byte. That is, even though sides is 1 character long, it really is 2 bytes: {5,'\0'}.

So it would be:

tempSides = (char *)malloc((strlen(sides)+1)*sizeof(char));

and if you wanna copy it in:

strcpy(tempSides, sides);
Claudiu
You mean '\0' when you say NULL. '\0' is nul, not NULL.
George Phillips
Note that multiplying by `sizeof(char)` is unneccesary; `sizeof(char)` is defined to be 1.
caf
@caf: True, but omitting it makes it harder to adapt code to wchar_t or TCHAR when necessary - if there're no multiplications there's a risk of forgetting to use one.
sharptooth
Don't forget to check for NULL before the `strcpy`
R Samuel Klatchko
Thanks for your suggestions...
iSight
@sharptooth: Multiplying by `sizeof *tempSides` would be best if you're concerned about switching to `wchar_t` later.
jamesdlin
@George Phillips: Strictly speaking, you mean 'null character'. `nul` is the ASCII moniker for the null character; the term 'nul' does not appear anywhere in the C standard.
dreamlax
@R Samuel Klatchko: how do you propose doing such a check, and what would the possible consequences be? If there's no `\0` after the input, both `strlen()` and `strcpy` would fail in the same way as your proposed check.
MSalters
@MSalters: he meant checking the `malloc` return value.
jweyrich
I'd use `strncpy` instead of `strcpy`. In this case we know that the buffer is big enough, so `strcpy` is OK. However, I prefer to avoid `strcpy` in ALL cases. The overhead is so tiny you are unlikely to see a real difference in performance.
daotoad
+1  A: 

There's a problem with that. tempSides will point to an uninitialized block of memory of size 1. If you intend to copy the sides string into tempSides, then you will need to allocate a size one byte longer, in order to hold the zero terminator for the string. The value returned by strlen() does not include the zero terminator at the end of the string.

Vagrant
A: 

No, not really. As others have already noted, you need to allocate space for the NUL terminator.

In addition, you generally should not cast the return from malloc. It can cover up a bug where you've forgotten to #include the correct header. Multiplying by sizeof(char) is also pointless, since the standards (both C and C++) define sizeof(char) to always be 1.

Finally, every call to malloc should include a test of the result. I'd wrap the whole thing up into a function:

char *dupe_string(char const *string) { 
    char *temp;
    if (NULL!=(temp=malloc(strlen(string)+1)))
        strcpy(temp, string);
    return temp;
}
Jerry Coffin
+2  A: 

As has been pointed out, you missed allocating space for the terminating NUL chararacter. But I also wanted to point out a couple of other things that can make your code more concise.

By definition, sizeof(char) is always 1, so you can shorten your allocation line to:

tempSides = (char*)malloc(strlen(inSides) + 1);

Another thing is that this looks like you are doing to duplicate the string. There is a built in function that does that for you:

tempSides = strdup(inSides);

This handles getting the length, allocating the correct number of bytes and copying the data.

R Samuel Klatchko
+3  A: 

Note that:

  1. Strings are zero-terminated (\0), and strlen() doesn't count it;
  2. By definition, sizeof(char) is 1 (byte), so it's not required;
  3. If you use a C (not C++) compiler, there's no need to cast it to char *;

So that would be:

char *tempSides = malloc(strlen(inSides) + 1);

Still, if you want to duplicate the contents of inSides, you can use strdup, e.g.:

char *tempSides = strdup(inSides);
if (tempSides != NULL) {
    // do whatever you want...
    free(tempSides);
}
jweyrich
A: 

Multiplying the element count by sizeof(char) is a matter of personal preference, since sizeof(char) is always 1. However, if you do this for consistency, better use the recipient pointer type to determine the element size, instead of specifying type explicitly. And don't cast the result of malloc

tempSides = malloc(strlen(inSides) * sizeof *tempSides);

Of course, when working with zero-terminated strings you have to remember to allocate extra space for the terminating zero character. There's no way to say whether it is your intent to make tempSides a zero-terminated string in this case, so I can't say whether you need it.

AndreyT
No way? He better should use zero-terminated strings when calling `strlen`... If he uses his own strlen, then I really don't want to be the maintenance programmer after him. ;)
Secure
@Secure: That only means that `inSides` is zero-terminated. There's no indication in the code that `tempSides` should be zero-terminated as well.
AndreyT