Thread: Memory leak in Soln8_05.cpp from Ivor Horton's Beginning Visual C++ 2008 book?

  1. #1
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63

    Question Memory leak in Soln8_05.cpp from Ivor Horton's Beginning Visual C++ 2008 book?

    Hi,

    I've been working through Ivor Horton's Beginning Visual C++ 2008 book and have made good progress so far. However, I've come across what looks to be a memory leak in one of the exercise solutions and I'm sure it's got to be me that's wrong.

    In chapter 8, exercises two through five you're asked:

    2. Implement a simple string class in native C++ that holds a char* and an integer length as private data members. Provide a constructor which takes an argument of type const char*, and implement the copy constructor, assignment operator and destructor functions. Verify that your class works. You will find it easiest to use the string functions from the <cstring> header file.

    3. What other constructors might you want to supply for your string class? Make
    a list, and code them up.

    4. (Advanced) Does your class correctly deal with cases such as this?
    string s1;
    ...
    s1 = s1;
    If not, how should it be modified?

    5. (Advanced) Overload the + and += operators of your class for concatenating strings.
    Attached is the author's solution to exercises 2 through 5. In Soln8_05.cpp, lines 91-100, you have the following:

    Code:
    // Addition operator: add two CSimpleString objects
    CSimpleString CSimpleString::operator+(const CSimpleString& s)
    {
      size_t length = len + s.len + 1;
       char* tmp = new char[length];
       strcpy_s(tmp, length, buff);
       strcat_s(tmp, length, s.buff);
    
       return CSimpleString(tmp);
    }

    Am I right in that the memory for tmp is never deleted?



    I thought that maybe since it was passing out of scope that it would be automatically "freed", but that only applies to automatic variables. Then I thought that maybe because it was a pointer to char* that it had static duration, but no, that applies to string literals.

    Thanks for any/all thoughts on this.
    Attached Files Attached Files
    Last edited by deoren; 01-09-2012 at 06:56 PM. Reason: Added missing attachment
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  2. #2
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    I believe that you're correct. tmp is not deleted. So the code should construct the object into a temporary CSimpleString variable, delete tmp, then return the object.

  3. #3
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,412
    Unless the CSimpleString object takes ownership of the memory allocated externally, which would be a strange thing to do here, yes, there is a memory leak.

    I note that the author's solution for operator+ is not const-correct: since operator+ does not modify the current object, it should be declared const.

    Incidentally, since you are already going to implement operator+=, you can use it to implement operator+ as non-member function:
    Code:
    // Addition operator: add two CSimpleString objects
    CSimpleString operator+(const CSimpleString& lhs, const CSimpleString& rhs)
    {
        return lhs += rhs;
    }
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  4. #4
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63

    Smile

    Thanks to both you you. I appreciate you confirming the leak and for providing a solution to the problem.

    I've been going through and comparing my code to the author's solution to see what I should have done differently, and examples like that really help.
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  5. #5
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    Quote Originally Posted by laserlight View Post
    Incidentally, since you are already going to implement operator+=, you can use it to implement operator+ as non-member function:
    Code:
    // Addition operator: add two CSimpleString objects
    CSimpleString operator+(const CSimpleString& lhs, const CSimpleString& rhs)
    {
        return lhs += rhs;
    }
    I went back over the code and I keep getting hung up on the behavior of operator+ in this case. If I'm understanding you right, in your example the existing operator+= member function is used. To my untrained eyes, it looks like lhs is being modified.

    Say you have three CSimpleString objects, str1, str2 and str3, all declared and initialized properly.

    If I were to do:

    Code:
    str1 = str2 + str3
    using the code you provided, wouldn't that modify str2?

    Edit:

    I went to test the code and got compile errors. I'm attaching the changed file and a diff between the author's original version, and my changes based on the code you suggested. I figure I'm overlooking something obvious.

    Compile log:

    Code:
    ClCompile:
      Soln8_05.cpp
    Soln8_05.cpp(105): error C2679: binary '+' : no operator found which takes a right-hand operand of type 'CSimpleString' (or there is no acceptable conversion)
              Soln8_05.cpp(24): could be 'CSimpleString CSimpleString::operator +(const char *)'
              while trying to match the argument list '(CSimpleString, CSimpleString)'
    
    Soln8_05.cpp(111): error C2679: binary '+' : no operator found which takes a right-hand operand of type 'const CSimpleString' (or there is no acceptable conversion)
              Soln8_05.cpp(24): could be 'CSimpleString CSimpleString::operator +(const char *)'
              while trying to match the argument list '(CSimpleString, const CSimpleString)'
    
    Soln8_05.cpp(123): error C2678: binary '+=' : no operator found which takes a left-hand operand of type 'const CSimpleString' (or there is no acceptable conversion)
              Soln8_05.cpp(22): could be 'CSimpleString &CSimpleString::operator +=(const CSimpleString &)'
              while trying to match the argument list '(const CSimpleString, const CSimpleString)'
    Thanks in advance!
    Attached Files Attached Files
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  6. #6
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,412
    Quote Originally Posted by deoren
    I went back over the code and I keep getting hung up on the behavior of operator+ in this case. If I'm understanding you right, in your example the existing operator+= member function is used. To my untrained eyes, it looks like lhs is being modified.
    Yes, the idea is to modify lhs and return a copy of it. Unfortunately, you cannot do that, because lhs is a const reference. To fix that, change it to either:
    Code:
    CSimpleString operator+(CSimpleString lhs, const CSimpleString& rhs)
    {
        return lhs += rhs;
    }
    or:
    Code:
    CSimpleString operator+(const CSimpleString& lhs, const CSimpleString& rhs)
    {
        return CSimpleString(lhs) += rhs;
    }
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  7. #7
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    Quote Originally Posted by laserlight View Post
    Yes, the idea is to modify lhs and return a copy of it. Unfortunately, you cannot do that, because lhs is a const reference.
    Okay, I think I understand you now. I should not implement your code from comment #3 because it is trying to modify a const object.

    While on that thought, if the code from post #3 worked, wouldn't that be modifying str2 in the following code?

    Code:
    str1 = str2 + str3;
    I know you can override the logic behind the operators when overloading them and that + doesn't have to mean addition, but when I read that statement I expect that str2 and str3 will remain untouched and only str1 will be changed.

    On a different note, I went back and reran the code from post 5 and I'm not getting the all of the previous compile errors, only this one:

    Code:
    soln8_05.cpp(176): error C2678: binary '+=' : no operator found which takes a left-hand operand of type 'const CSimpleString' (or there is no acceptable conversion)
    soln8_05.cpp(25): could be 'CSimpleString &CSimpleString::operator +=(const CSimpleString &)'
    I also forgot to mention that I'm using Microsoft Visual Studio 2010 SP1 to compile the code.

    I reverted back to the author's code, then made the changes you mentioned in the first code block of post 6 and got this error:

    Code:
    First-chance exception at 0x7c90e8e5 in Soln8_05.exe: 0xC00000FD: Stack overflow.
    First-chance exception at 0x7c812afb in Soln8_05.exe: Microsoft C++ exception: std::bad_alloc at memory location 0x00033680..
    Unhandled exception at 0x7c812afb in Soln8_05.exe: Microsoft C++ exception: std::bad_alloc at memory location 0x00033680..
    I've been taking your code examples and have pretty much dropped them in as-is, aside from adding the function prototype immediately following the class definition's closing curly brace and semicolon. The only other change has been removing the author's original code that yours replaces.

    The second example gives the same error.

    Could you look over the attached file and let me know what I'm doing wrong? It contains your second suggestion.

    Thanks for your time.
    Attached Files Attached Files
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  8. #8
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    You're defining operator+= and operator+ each in terms of the other! You have to define at least one of them to be able to do its work without recourse to the other.

    BTW, the person's username who helped you before is "laserlight" not "lightning". And you're not really "adding" two strings but "concatenating" them.


    PS: For anyone else trying to run this code, here's a couple of defines I added to the top to get around the non-standard functions:
    Code:
    #define strcpy_s(a,b,c) strcpy(a,c)
    #define _itoa_s(a,b,c,d) itoa(a, b, d)

  9. #9
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,412
    Quote Originally Posted by deoren
    I should not implement your code from comment #3 because it is trying to modify a const object.
    Yes.

    Quote Originally Posted by deoren
    While on that thought, if the code from post #3 worked, wouldn't that be modifying str2 in the following code?
    It does not work and trying to reason what would happen if it worked is futile.

    Quote Originally Posted by deoren
    I know you can override the logic behind the operators when overloading them and that + doesn't have to mean addition, but when I read that statement I expect that str2 and str3 will remain untouched and only str1 will be changed.
    Yes.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  10. #10
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    Quote Originally Posted by oogabooga View Post
    BTW, the person's username who helped you before is "laserlight" not "lightning".
    My apologies to laserlight on the mix-up. I knew a lightning from another forum and somehow made the (wrong) association on the name.


    Quote Originally Posted by oogabooga View Post
    And you're not really "adding" two strings but "concatenating" them.
    I used the wrong terminology, but we're on the same page. I meant to convey that I associate the operator+() function with not changing the objects (str2 and str3 in my previous examples) and the operator+=() function with making changes.

    Thanks for the macros.

    Quote Originally Posted by oogabooga View Post
    You're defining operator+= and operator+ each in terms of the other! You have to define at least one of them to be able to do its work without recourse to the other.
    I was a little confused by that as well, but I was trying to take laserlight's code as-is and use it in place of the author's implementation. At this point, I have to admit I'm pretty confused.

    I have my solution (which isn't near the quality as the author's, but works) and then I have the author's solution (Soln8_05.cpp, r1) where I noticed the leak after comparing my solution (not shown) with the author's.

    oogabooga, you then responded with post #2, so I made the changes shown in (Soln8_05.cpp, r2) where I attempted to implement your suggested fix. It seems to work from what I can tell.

    Then I saw laserlight's post and tried to implement the provided code as
    (Soln8_05.cpp, r3).

    I failed to do so, and received the errors mentioned in post #5.

    laserlight responded again (thanks by the way, I do appreciate all help) on post #6 and provided more code. Based on the wording, it was an either/or approach so I implemented the first suggestion separately as (Soln8_05.cpp, r4) and the second suggestion as (Soln8_05.cpp, r5).

    More errors, and likely because of how I implemented the provided code. I posted my results on post #7.

    I reverted back to r2, oogabooga, and I've now included your macros as (Soln8_05.cpp, r6).

    laserlight:

    Are any of your code examples supposed to be usable as-is, or do I need to modify existing code to make it work? Please use (Soln8_05.cpp, r6) to compare against.

    Thanks in advance.

    P.S.

    I'm using a public SVN repo of mine to provide code. I'm hoping this helps to make my thoughts clearer.
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  11. #11
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,412
    Quote Originally Posted by deoren
    Are any of your code examples supposed to be usable as-is, or do I need to modify existing code to make it work?
    If you defined operator+= correctly, without defining it in terms of the overloaded operator+, then yes, my examples in post #6 should work as is, assuming there are no problems with the copy constructor and/or destructor.

    Also, I am assuming that the overloaded operator+= is like:
    Code:
    CSimpleString& CSimpleString::operator+=(const CSimpleString& other)
    {
        // ...
        return *this;
    }
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  12. #12
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    Quote Originally Posted by laserlight View Post
    Yes.


    It does not work and trying to reason what would happen if it worked is futile.


    Yes.
    Thanks for the reply. I took forever to reply and by the time I did you had already responded.

    I was referring more to a high-level pseudocode sort of view. I didn't understand the example of using += to modify an existing object inside of an overloaded + operator function, which is what I took this code to mean:

    Quote Originally Posted by laserlight View Post
    Incidentally, since you are already going to implement operator+=, you can use it to implement operator+ as non-member function:
    Code:
    // Addition operator: add two CSimpleString objects
    CSimpleString operator+(const CSimpleString& lhs, const CSimpleString& rhs)
    {
        return lhs += rhs;
    }
    To expand on that, I interrupted the function as meaning:

    1. The non-member function receives two objects (as const, read-only), lhs and rhs
    2. An attempt to modify lhs in place to include rhs is made
    3. Return the modified lhs object
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  13. #13
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    Quote Originally Posted by laserlight View Post
    If you defined operator+= correctly, without defining it in terms of the overloaded operator+, then yes, my examples in post #6 should work as is, assuming there are no problems with the copy constructor and/or destructor.

    Also, I am assuming that the overloaded operator+= is like:
    Code:
    CSimpleString& CSimpleString::operator+=(const CSimpleString& other)
    {
        // ...
        return *this;
    }
    In the author's original code, it's defined like so:

    Code:
    // += operator
    CSimpleString& CSimpleString::operator+=(const CSimpleString& rhs)
    {
       *this = *this + rhs;
       return *this;
    }
    Which if I'm understanding you correctly, means the overloaded += function is defined in terms of the overloaded operator+ function. It appears we're on the same page now.

    Thanks for your patience. I feel much better about my understanding of the code/situation now.

    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  14. #14
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,412
    Quote Originally Posted by deoren
    Which if I'm understanding you correctly, means the overloaded += function is defined in terms of the overloaded operator+ function. It appears we're on the same page now.
    Ah yes. That is less common because operator+= modifies the left hand side argument, so there is no need for another object to be created, hence it is potentially more efficient than operator+. Implementing operator+= in terms of operator+ makes it no more efficient than operator+, which is a net loss.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  15. #15
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    Quote Originally Posted by laserlight View Post
    operator+= modifies the left hand side argument
    That makes sense to me. The rest, not so much. I'll take your word for it until I learn more and have a chance to see it in action. This particular chapter is making my head hurt.


    Quote Originally Posted by oogabooga View Post
    I believe that you're correct. tmp is not deleted. So the code should construct the object into a temporary CSimpleString variable, delete tmp, then return the object.
    Keeping with the author's original code, I implemented your suggestion (and macros) in (Soln8_05.cpp, r6) as:

    Code:
       CSimpleString temp_obj(tmp);
       delete [] tmp;
       return CSimpleString(temp_obj);
    but now I'm wondering if I did it wrong.

    It works, but instead of having:

    Code:
       return CSimpleString(temp_obj);
    should I have

    Code:
       return temp_obj;
    instead?

    Since I'm not returning a pointer, but an object by value, I'm thinking the second approach is fine?

    Thanks in advance for any help.
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 10
    Last Post: 11-27-2011, 11:44 AM
  2. Detecting Memory leak in Visual Studio (Debug mode)
    By elizas in forum Windows Programming
    Replies: 3
    Last Post: 05-06-2010, 11:14 AM
  3. how to deteck memory leak in visual c++?
    By franziss in forum Windows Programming
    Replies: 16
    Last Post: 11-11-2009, 07:57 PM
  4. answers for Ivor Horton's Beginning C++?
    By 7stud in forum C++ Programming
    Replies: 1
    Last Post: 03-17-2005, 12:13 AM
  5. "Ivor Horton's Beginning ANSI C++" Wacha think?
    By Zeusbwr in forum C++ Programming
    Replies: 0
    Last Post: 10-24-2004, 07:30 PM

Tags for this Thread