-
January 22nd, 2010, 12:58 PM
#1
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.
-
January 22nd, 2010, 01:05 PM
#2
Re: What is the problem with the code?
What's the point in writing a string class if it has a fixed maximum size (1024) anyway?
-
January 22nd, 2010, 01:20 PM
#3
Re: What is the problem with the code?
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:
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.
Vlad - MS MVP [2007 - 2012] - www.FeinSoftware.com
Convenience and productivity tools for Microsoft Visual Studio:
FeinWindows - replacement windows manager for Visual Studio, and more...
-
January 22nd, 2010, 01:21 PM
#4
Re: What is the problem with the code?
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.
-
January 22nd, 2010, 01:29 PM
#5
Re: What is the problem with the code?
And rule of three is missing.
-
January 22nd, 2010, 01:36 PM
#6
Re: What is the problem with the code?
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.
-
January 22nd, 2010, 04:29 PM
#7
Re: What is the problem with the code?
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.
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:
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.
-
January 22nd, 2010, 04:42 PM
#8
Re: What is the problem with the code?
Code:
MyString foo("Some string goes here");
MyString bar;
bar = foo;
Crash...
-
January 22nd, 2010, 06:33 PM
#9
Re: What is the problem with the code?
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.
-
January 26th, 2010, 11:43 AM
#10
Re: What is the problem with the code?
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;
}
-
January 26th, 2010, 12:20 PM
#11
Re: What is the problem with the code?
MyString operator+(const char* a, const MyString& b)
0100 0111 0110 1111 0110 0100 0010 0000 0110 1001 0111 0011 0010 0000 0110 0110 0110 1111 0111 0010
0110 0101 0111 0110 0110 0101 0111 0010 0010 0001 0010 0001 0000 0000 0000 0000 0000 0000 0000 0000
-
January 26th, 2010, 12:22 PM
#12
Re: What is the problem with the code?
Check this FAQ, it has quite bit of info relevant for you.
-
January 26th, 2010, 12:24 PM
#13
Re: What is the problem with the code?
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.
Originally Posted by Joeman
MyString operator+(const char* a, const MyString& b)
Last edited by dullboy; January 26th, 2010 at 12:33 PM.
-
January 26th, 2010, 12:27 PM
#14
Re: What is the problem with the code?
Code:
MyString s1("Hi");
MyString s2;
s2 = MyString("there ") + s1;
-
January 26th, 2010, 12:30 PM
#15
Re: What is the problem with the code?
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:
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.
Last edited by laserlight; January 26th, 2010 at 12:39 PM.
Posting Permissions
- You may not post new threads
- You may not post replies
- You may not post attachments
- You may not edit your posts
-
Forum Rules
|
Click Here to Expand Forum to Full Width
|