What is the problem with the code?

Here is my code,

Code:

---

class MyString
{
public:
        MyString(const char* str)
        {
                m_str = new char[strlen(str)+1];
                strcpy_s(m_str,strlen(str)+1, str);
        }

        MyString()
        {
                m_str = new char[1024];
        }

        ~MyString()
        {
                delete[] m_str;
        }

        friend MyString operator+(MyString& a, const MyString& b);
    friend ostream& operator<<(ostream& os, MyString& str);
       
private:
        char* m_str;
};

MyString operator+(MyString& a, const MyString& b)
{
        strcat_s(a.m_str, strlen(a.m_str) + strlen(b.m_str) + 1, b.m_str);
        return a.m_str;
}

ostream& operator<<(ostream& os, MyString& str)
{
        os<<str.m_str;
        return os;
}

int _tmain(int argc, _TCHAR* argv[])
{
        MyString s1("Hi");
        MyString s2;

        s2 = s1 + " there";

        cout<<s2<<endl;
        return 0;
}

---

Could you point out the problems? Thanks for your inputs.

What's the point in writing a string class if it has a fixed maximum size (1024) anyway?

Quote:

---

Originally Posted by dullboy

Could you point out the problems? Thanks for your inputs.

---

One obvious problem is that secure version of strcat() requires the available size of destination string to prevent buffer overrun; you are passing the “desired” size, which no one allocates.
In your main function, the constructor:

Code:

---

s1("Hi");

---

will allocate exactly 3 chars, with no room for concatenation. So this line:

Code:

---

s2 = s1 + " there";

---

will write outside of allocated memory (undefined behavior; likely – crush).
Also, don’t you need to implement an assignment operator for your string? The one generated by compiler will copy your member variables (char* m_str). When the original string object is deleted, your “copied” one will contain a dead pointer.

I'm curious as to why you're writing your own string class anyway. std::string is very stable, and if speed is your concern, there are many replacements out there that you're not going to be able to beat without boatloads of assembly.

And rule of three is missing.

1) As already pointed out, you need to implement the assignment operator

2) you also need to implement the copy constructor

3) the second argument to operator << should be a const reference

4) As also pointed out, operator + does not check if there is enough room
to do the concatenation. And you do not have a way to determine how
much room there actually is. In your example code, s2 will have 1024,
but s1 will only have 3.

Thanks for your reponse! You said " When the original string object is deleted, your “copied” one will contain a dead pointer". I wander when that the original string object is deleted happens? It looks like no variables are out of scope. Thanks for your inputs.

Quote:

---

Originally Posted by VladimirF

One obvious problem is that secure version of strcat() requires the available size of destination string to prevent buffer overrun; you are passing the “desired” size, which no one allocates.
In your main function, the constructor:

Code:

---

s1("Hi");

---

will allocate exactly 3 chars, with no room for concatenation. So this line:

Code:

---

s2 = s1 + " there";

---

will write outside of allocated memory (undefined behavior; likely – crush).
Also, don’t you need to implement an assignment operator for your string? The one generated by compiler will copy your member variables (char* m_str). When the original string object is deleted, your “copied” one will contain a dead pointer.

---

Code:

---

MyString foo("Some string goes here");
MyString bar;
bar = foo;

---

Crash...

Thanks for your valuable suggestions, here is my revised code.

Code:

---

class MyString
{
public:
        MyString& operator=(const MyString& rhs)
        {
                if(this != &rhs)                       
                        strcpy_s(m_str,strlen(rhs.m_str)+1, rhs.m_str);

                return *this;
        }

        MyString(const MyString& rhs)
        {
                m_str = new char[strlen(rhs.m_str)+1];
                strcpy_s(m_str,strlen(rhs.m_str)+1, rhs.m_str);
        }

        MyString(const char* str)
        {
                m_str = new char[strlen(str)+1];
                strcpy_s(m_str,strlen(str)+1, str);
        }

        MyString()
        {
                m_str = new char[1024];
        }

        ~MyString()
        {
                delete[] m_str;
        }

        friend MyString operator+(MyString& a, const MyString& b);
    friend ostream& operator<<(ostream& os, const MyString& str);
       
private:
        char* m_str;
};

MyString operator+(MyString& a, const MyString& b)
{
        int nLen1 = strlen(a.m_str);
        int nLen2 = strlen(b.m_str);

        char* tem = new char[nLen1+1];
        strcpy_s(tem, nLen1+1, a.m_str);

        delete[] a.m_str;

        a.m_str = new char[nLen1+nLen2+1];
        strcpy_s(a.m_str, strlen(tem)+1, tem);

        strcat_s(a.m_str, nLen1+nLen2+1, b.m_str);

        delete[] tem;
        return a.m_str;
}

ostream& operator<<(ostream& os, const MyString& str)
{
        os<<str.m_str;
        return os;
}

int _tmain(int argc, _TCHAR* argv[])
{
        MyString s1("Hi");
        MyString s2;
 
        s2 = s1 + " there";

        cout<<s2<<endl;
        return 0;
}

---

If I comment out my implementation of assignment operator, then program will crash on the line of s2=s1+" there". I understand without assignment operator overloading, default assignment operator only does shallow copy. As the result, two pointers(m_str) will point to same address. But at the moment of calling s2 = s2 + " there", it looks like no pointer is deleted. So why does it crash there?
Also is there any improvement on my code? Thanks for your inputs.

If I use the class defined below by calling,
MyString s1("Hi");
MyString s2;
s2 = "there " + s1;

Then it doesn't work. How can I make s2="there " + s1 possible? Thanks for your inputs.

Code:

---

class MyString
{
public:
        MyString& operator=(const MyString& rhs)
        {
                if(this != &rhs)                       
                        strcpy_s(m_str,strlen(rhs.m_str)+1, rhs.m_str);

                return *this;
        }

        MyString(const MyString& rhs)
        {
                m_str = new char[strlen(rhs.m_str)+1];
                strcpy_s(m_str,strlen(rhs.m_str)+1, rhs.m_str);
        }

        MyString(const char* str)
        {
                m_str = new char[strlen(str)+1];
                strcpy_s(m_str,strlen(str)+1, str);
        }

        MyString()
        {
                m_str = new char[1024];
        }

        ~MyString()
        {
                delete[] m_str;
        }

        friend MyString operator+(MyString& a, const MyString& b);
    friend ostream& operator<<(ostream& os, const MyString& str);
       
private:
        char* m_str;
};

MyString operator+(MyString& a, const MyString& b)
{
        int nLen1 = strlen(a.m_str);
        int nLen2 = strlen(b.m_str);

        char* tem = new char[nLen1+1];
        strcpy_s(tem, nLen1+1, a.m_str);

        delete[] a.m_str;

        a.m_str = new char[nLen1+nLen2+1];
        strcpy_s(a.m_str, strlen(tem)+1, tem);

        strcat_s(a.m_str, nLen1+nLen2+1, b.m_str);

        delete[] tem;
        return a.m_str;
}

ostream& operator<<(ostream& os, const MyString& str)
{
        os<<str.m_str;
        return os;
}

---

MyString operator+(const char* a, const MyString& b)

Check this FAQ, it has quite bit of info relevant for you.

I don't want to change the interface of operator+ because the way operator+ takes char* as an argument violates intuition. I use operator to enhance the intuition. Any other ideas? Thanks for your inputs.

Quote:

---

Originally Posted by Joeman

MyString operator+(const char* a, const MyString& b)

---

Code:

---

MyString s1("Hi");
MyString s2;
s2 = MyString("there ") + s1;

---

Quote:

---

Originally Posted by dullboy

I don't want to change the interface of operator+.

---

Why not? It looks like you are giving operator+ the semantics of operator+=, which is a Bad Thing. Furthermore, your implementation of operator+ does unnecessary work: you should just create a buffer for the result, copy over what is necessary, then destroy a's contents and then make a's pointer point to the start of the new result buffer. (Of course, this would be after you have changed this to operator+=.)

EDIT:

Quote:

---

Originally Posted by dullboy

because the way operator+ takes char* as an argument violates intuition. I use operator to enhance the intuition.

---

Joeman's suggestion will allow expressions such as: "hello" + str. I do not see how that violates intuition. But what I had in mind was changing your current operator+ to take its first argument by const reference. This would also allow expressions such as "hello" + str, except that it will likely be less efficient than providing an overload of operator+ with a const char* first parameter.

Yes, Joeman's suggestion does allow expressions such as: "hello" + str, but the definition of operator+ will expose char* to clients. I just don't like this idea. Basically my goal is that clients shouldn't see char*. I expect that you may pass char* to my original defintion of operator + and at the mean time char* will be IMPLICITLY converted to MyString. But obviously when a char* is on the left hand side of +, it failed to be converted to MyString. That is my question. How'd I make it possible. Thanks for your inputs.

Quote:

---

Originally Posted by laserlight

Why not? It looks like you are giving operator+ the semantics of operator+=, which is a Bad Thing. Furthermore, your implementation of operator+ does unnecessary work: you should just create a buffer for the result, copy over what is necessary, then destroy a's contents and then make a's pointer point to the start of the new result buffer. (Of course, this would be after you have changed this to operator+=.)

EDIT:

Joeman's suggestion will allow expressions such as: "hello" + str. I do not see how that violates intuition. But what I had in mind was changing your current operator+ to take its first argument by const reference. This would also allow expressions such as "hello" + str, except that it will likely be less efficient than providing an overload of operator+ with a const char* first parameter.

---

Quote:

---

Originally Posted by dullboy

but the definition of operator+ will expose char* to clients.

---

What do you mean by that?

Quote:

---

Originally Posted by dullboy

I expect that you may pass char* to my original defintion of operator + and at the mean time char* will be IMPLICITLY converted to MyString.

---

Yes, and as I noted this would be possible if you changed the declaration to:

Code:

---

MyString operator+(const MyString& a, const MyString& b);

---

Note that the first parameter is also a const reference. Alternatively, the first parameter could be a value, so you could write:

Code:

---

MyString operator+(MyString a, const MyString& b)
{
    return a += b;
}

---

Making use of operator+=, which you would overload yourself to have an implementation similiar to what you currently have for operator+.

Quote:

---

Originally Posted by dullboy

But obviously when a char* is on the left hand side of +, it failed to be converted to MyString.

---

That is the whole idea behind adding an additional overhead (or two) involving const char* parameters: you want to avoid the implicit conversion to MyString, as that has a cost attached to it.

If I change first parameter of operator+ to a const, then it doesn't compile because I need to modify it within body of operator+. Thanks for your inputs.

Quote:

---

Originally Posted by laserlight

What do you mean by that?


Yes, and as I noted this would be possible if you changed the declaration to:

Code:

---

MyString operator+(const MyString& a, const MyString& b);

---

Note that the first parameter is also a const reference. Alternatively, the first parameter could be a value, so you could write:

Code:

---

MyString operator+(MyString a, const MyString& b)
{
    return a += b;
}

---

Making use of operator+=, which you would overload yourself to have an implementation similiar to what you currently have for operator+.


That is the whole idea behind adding an additional overhead (or two) involving const char* parameters: you want to avoid the implicit conversion to MyString, as that has a cost attached to it.

---

Quote:

---

Originally Posted by dullboy

If I change first parameter of operator+ to a const, then it doesn't compile because I need to modify it within body of operator+.

---

That's right. operator+ should not modify its first argument. Rather, you should create an operator+= instead, and use that to implement operator+.

Put it another way: a = 2, b = 3. c = a + b. What is the value of a? According to you, the value of a is now 5. Does that make sense to you?