How should I allocate memory for c-string char array?

Question

So in attempting to learn how to use C-Strings in C++, I'm running into issues with memory allocation.

The idea here is that a new string is created of the format (s1 + sep + s2) The text I'm using provided the header, so I can't change that, but I'm running into issues trying to set the size of char str[]. I am getting an error saying that sLength is not constant, and therefore cannot be used to set the size of an array. I'm relatively new to C++ so this is a two part question.

1. Is this strategy actually allocating memory for the new array?

2. How do I set the array size correctly if I can't get a constant value using strlen(char*)?

   char* concatStrings(char* s1, char* s2, char sep){
       int sLength = strlen(s1) + strlen(s2) + 3; 
       //+1 for char sep +2 for \0 at end of string
       char *str = new char[sLength];
       strcpy (str, s1);
       str [sLength(s1)] = sep;
       strcat (str, s2);
       return str;
   }
   

Edits made, so now I'm getting no compiler errors but...

The call to the function is here:

    char* str = concatStrings("Here is String one", "Here is String two" , c);
    cout<< str;

My output becomes:

Here is String onec==================22221/21/21/21/2 /(etc.)/ Here is String two

Damalis

You have to dynamically allocate it. If you're new to C++, I suggest sticking to std::string for a while before looking into C strings. Don't pick C strings over std::string to use, as there are all kinds of problems. – Garble

What compiler errors do you get? – Rectory

@Garble In general, I agree with the suggestion to prefer std::string over C strings. However, the OP clearly states that this is a learning exercise which is a perfectly valid reason (IMO) to use C strings. – Rectory

Personally, if I wanted to learn about C strings in C++, I would write my own implementation of std::string. It's a fantastic example of RAII and just a class done right... – Bibelot

@Code-Guru, I recognize that, but I had two other things in mind. Firstly, the OP said relatively new to C++, and doing C strings before recognizing the need for DMA caught my attention. Secondly, my note at the end was just that: a notice of the superior option that should be used in any real code. – Garble

The code in this question probably should not have been changed after it was answered. – Scotia

Answer 1

Error is returning address of local array variable str. Its scope is within function concatStrings() where you declared, and can't be accessed once control returns from the function.

To access it outside, you need to dynamically allocate memory for the string from the heap using the new operator.

char* concatStrings(char* s1, char* s2, char sep){
    int s1Length = strlen(s1);
    int sLength = s1Length + strlen(s2) + 2; 
    // +1 for sep and +1 \0 at end of string
    char* str = new char[sLength];
    strcpy (str, s1);
    // Use strlen here instead of sizeof()
    str [s1Length] = sep;
    str [s1Length + 1] = '\0';
    strcat (str, s2);
    return str;
}

And after the program is done using the string returned from concatStrings it should ensure to free up the memory by invoking delete

char* str = concatStrings(s1, s2, sep);

// Do something

// Free up memory used by str
delete[] str; 

Must use delete[] here instead of delete, or it results in undefined behaviour

I've also edited the concatStrings() function to use strlen instead of sizeof

UPDATE: Thanks for pointing out that we only need to do +2 and not +3 and for making sure a '\0' needs to be appended after str1 and sep before invoking strcat

Latchkey

Good answer, though strlen(s1) should really be cached; it is O(N) after all... – Bibelot

@gnome : strcat() add \0 it self you don't need explicitly – Pagano

@Latchkey did you notice? what Gnome wanted to say. – Pagano

@GrijeshChauhan, and everyone else - I've incorporated all of the suggested changes. Yes realized that strcat requires that the two input strings be null terminated. – Latchkey

Everything works wonderfully now. Thank you so much. I'm now going to take a long look at a comparison to figure out what exactly was resulting in my odd results, but you have helped me greatly. So thank you. – Damalis

I've not touched C++ since the mid-90's, but need to get back in to the swing.. but IIRC, shouldn't the 'delete str' be 'delete [] str', since the string was allocated with new[] ? – Bali

Answer 2

You can allocate the resulting string memory dynamically (at run-time, on the heap), using new[] in C++ (or malloc for a more C-like style):

char* concatStrings(const char* s1, const char* s2, char sep) // enforced const correctness
{
    const size_t totalLength = strlen(s1) + strlen(s2) 
                            + 2; // +1 for sep char, +1 for '\0' 

    // Dynamically allocate room for the new string (on the heap)
    char* str = new char[totalLength];    

    strcpy(str, s1);
    str[strlen(s1)] = sep; // note that you had a typo with sizeof(s1) here
    strcat(str, s2);
    return str;
}

Note that this memory must be released somewhere in your code, using delete[] if it was allocated with new[], or free() if it was allocated using malloc().

This is quite complicated.

You will simplify your code a lot if you use a robust C++ string class like std::string, with its convenient constructors to allocate memory, destructor to automatically free it, and operator+ and operator+= overloads to concatenate strings. See how your code is simplified using std::string:

#include <string> // for std::string

std::string str = s1;
str += sep;
str += s2;

(Note that using raw C strings can also make your code more vulnerable to safety problems, since you must pay lot of attention to proper sizing destination strings, avoid buffer overruns, etc. This is another reason to prefer a RAII robust string class like std::string.)

Kawasaki

Answer 3

sizeof(s1) returns the size of a pointer variable, not the length of the array which it points to. Since you know that s1 points to a C-string, you should use the strlen() function instead.

Rectory

Thanks for that, I originally was using sizeof() for all of it, and meant to change all of them, missed one. – Damalis

@KurtVonDaimondorf There are other issues with your code. Before I expand on my answer, can you please edit your question with the compiler errors that you get? – Rectory

yesterday I edit two answers and you were present on both form. – Pagano