CodeGuru Home VC++ / MFC / C++ .NET / C# Visual Basic VB Forums Developer.com
Page 1 of 2 12 LastLast
Results 1 to 15 of 19
  1. #1
    Join Date
    Aug 2000
    Posts
    1,471

    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.

  2. #2
    Lindley is offline Elite Member Power Poster
    Join Date
    Oct 2007
    Location
    Seattle, WA
    Posts
    10,895

    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?

  3. #3
    Join Date
    Aug 2000
    Location
    New York, NY, USA
    Posts
    5,656

    Re: What is the problem with the code?

    Quote Originally Posted by dullboy View Post
    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.
    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...

  4. #4
    Join Date
    Jan 2009
    Posts
    1,689

    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.

  5. #5
    Join Date
    May 2007
    Posts
    811

    Re: What is the problem with the code?

    And rule of three is missing.

  6. #6
    Join Date
    Aug 2000
    Location
    West Virginia
    Posts
    7,721

    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.

  7. #7
    Join Date
    Aug 2000
    Posts
    1,471

    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.
    Quote Originally Posted by VladimirF View Post
    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.

  8. #8
    Join Date
    May 2007
    Posts
    811

    Re: What is the problem with the code?

    Code:
    MyString foo("Some string goes here");
    MyString bar;
    bar = foo;
    Crash...

  9. #9
    Join Date
    Aug 2000
    Posts
    1,471

    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.

  10. #10
    Join Date
    Aug 2000
    Posts
    1,471

    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;
    }

  11. #11
    Join Date
    Jun 2008
    Posts
    592

    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

  12. #12
    Join Date
    May 2007
    Posts
    811

    Re: What is the problem with the code?

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

  13. #13
    Join Date
    Aug 2000
    Posts
    1,471

    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.
    Quote Originally Posted by Joeman View Post
    MyString operator+(const char* a, const MyString& b)
    Last edited by dullboy; January 26th, 2010 at 12:33 PM.

  14. #14
    Join Date
    May 2007
    Posts
    811

    Re: What is the problem with the code?

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

  15. #15
    Join Date
    Jan 2006
    Location
    Singapore
    Posts
    6,765

    Re: What is the problem with the code?

    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.
    Last edited by laserlight; January 26th, 2010 at 12:39 PM.
    C + C++ Compiler: MinGW port of GCC
    Build + Version Control System: SCons + Bazaar

    Look up a C/C++ Reference and learn How To Ask Questions The Smart Way
    Kindly rate my posts if you found them useful

Page 1 of 2 12 LastLast

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  





Click Here to Expand Forum to Full Width

Featured