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:

Quote:

---

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?

:confused:

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.

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.

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

---

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.

Quote:

---

Originally Posted by laserlight

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!

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 laserlight

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.

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)

---

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 oogabooga

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

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

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.

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 laserlight

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

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

Quote:

---

Originally Posted by laserlight

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.

:)

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 laserlight

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

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.