Run time error or memory leak problem?

[MisterTams]

Hey guys,

Just as the title states. I don't know where I went wrong. The command prompt window will come up and display my information but then a dialog box comes up saying "Debug Assertion Failed!" How can I go about fixing this? I bet there is a small error somewhere in my code that I can't seem to find....

Here are the files...

Circle.h

// using declarations for std namespace
#include <iostream>
#include <iomanip>
#include <string>
using namespace std;

// prevent multiple definitions of Student class
#ifndef CIRCLE_H
#define CIRCLE_H

class Circle 
 {
 private:
	 string* name;
	 double diameter;
	 int centerX;
	 int centerY;
	 static int numObj;  // static data shared by all objects
 public:
	 // constructor
	 Circle();
	 Circle(string, double, int, int);
	 // copy constructor
	 Circle(const Circle&);
	 // destructor
	 ~Circle();
	 // inline accessors
	 string getName() const {return *name;}
	 double getDiameter() const {return diameter;}
	 int getCenterX() const {return centerX;}
	 int getCenterY() const {return centerY;}
	 // mutators
	 void setName(string);
	 void setDiameter(double);
	 void setCenterX(int);
	 void setCenterY(int);
	 // calculators
	 double calcArea() const;
	 double calcPerimeter() const;
	 // overloaded insertion operator as friend function
	 friend ostream& operator<< (ostream&, const Circle&);
	 // static method to return the number of created objects
	 static int getNumObj();

	 // operator overloaded as member function
	 void operator = (const Circle &rhs);
 };
#endif 

Circle.cpp

#define _USE_MATH_DEFINES  // enable non-standard mathematical constants
#include <cmath>

// include specification file
#include "L10_Circle.h"

// constructors
Circle::Circle() {
	name = new string("");
	diameter = 0; 
	centerX = 0; 
	centerY = 0; 
	numObj++;
}

Circle::Circle(string n, double d, int x, int y) { 
	name = new string(n);
	diameter = d; 
	centerX = x; 
	centerY = y; 
	numObj++;
}

// copy constructor

Circle::Circle(const Circle& obj) 
{
	name = new string(*obj.name);
	diameter = obj.diameter;
	centerX = obj.centerX;
	centerY = obj.centerY;
	numObj++; // increment number of objects created
}

// destructor
Circle::~Circle() {
	delete name;
	numObj--;
}

// mutators
void Circle::setName(string n) {
	*name = n;
}

void Circle::setDiameter(double d) {
	diameter = d;
}

void Circle::setCenterX(int x) {
	centerX = x;
}

void Circle::setCenterY(int y) {
	centerY = y;
}

// calculators
double Circle::calcArea() const {
	return M_PI * pow((diameter / 2), 2.0);
}

double Circle::calcPerimeter() const {
	return M_PI * diameter;
}

int Circle::numObj = 0;

int Circle::getNumObj()
{
	return numObj;
}

// overloaded insertion operator as friend function
ostream& operator<< (ostream& lhs, const Circle& rhs) {
	lhs << *rhs.name << " diameter = " << fixed << setprecision(2) << rhs.diameter
		<< " area = " << rhs.calcArea() << endl;
	return lhs;
}

// operator overloaded as member function
void Circle::operator = (const Circle &rhs)
{
	//copy over values
	name = rhs.name;
	diameter = rhs.diameter;
	centerX = rhs.centerX;
	centerY = rhs.centerY;
}

#include "L10_Circle.h"

int main() {
	// output number of circle objects
	cout << "Number of circle objects before declarations: " << Circle::getNumObj() << endl;
	// create circle objects
	Circle c1("c1", 4.0, 1, 1), c2(c1), c3;
	// reset c2 properties
	c2.setName("c2");
	c2.setDiameter(6.0);
	// assign c2 properties to c3
	c3 = c2;
	// reset c3 properties
	c3.setName("c3");
	c3.setDiameter(8.0);
	// display circles
	cout << "**Circles***" << endl;
	cout << c1 << c2 << c3;
	// output number of circle objects
	cout << "Number of circle objects after declarations: " << Circle::getNumObj() << endl;
	return 0;
}
/*SAMPLE OUTPUT
Number of circle objects before declarations: 0
**Circles***
c3 diameter = 4.00 area = 12.57
c3 diameter = 6.00 area = 28.27
c3 diameter = 8.00 area = 50.27
Number of circle objects after declarations: 3
*/

[keskiverto]

Line 85 in Circle.cpp is inconsistent with the rest.


Why is the Circle::name a pointer? Why not a std::string?

[Moschops]

Your assignment operator is giving the newly created Circle object the exact same pointer value as the one it's copying, so you end up with two Circle objects using the same pointer to a string and thus their destructors call delete twice on the same pointer. This is bad.

As keskiverto suggests, in this case just use a string rather than a pointer to a string.

[MisterTams]

I totally understand that using a regular string would be easier. My requirement is to use name as a pointer. So again, how can I remedy this error when I run the program?

Does line 85 need to be this?? -> name = new string(*obj.name);

[mbozzi]

Does line 85 need to be this?? -> name = new string(*obj.name);

That's more correct, but still wrong; you're leaking the old name.

In any event, you should use your destructor and copy constructor to do the work for you:
See this post
http://www.cplusplus.com/forum/beginner/210637/#msg988290

I'd also be remiss if I didn't mention the copy-and-swap idiom, which stops you from losing data in the case of an exception:
https://chara.cs.illinois.edu/sites/cgeigle/blog/2014/08/27/copy-and-swap/
http://www.gotw.ca/gotw/059.htm

[MisterTams]

mbozzi,

I read that and it didn't help me much.
Can you just show me what I need to do for my particular code? I bet it's just like 2 lines of statements in total or something simple. I just can't seem to get it.

[mbozzi]

Take the second snippet of code in that post and replace SortListClass with Circle.

That's it.

[Moschops]

If the requirement is that you simply must use a pointer to access the name...

class Circle 
 {
 private:
         string* pointerToName;
	 string actualName;
	 double diameter;
	 int centerX;
	 int centerY;
	 static int numObj;  // static data shared by all objects
 public:

and in each constructor

pointerToName = &actualname;

Using a pointer, no dynamic memory to worry about.

[mbozzi]

Just to note, such an object is an exception to the rule of three. (No nontrivial destructor is required.)

[JLBorges]

If the requirement is that you simply must use a pointer to access the name...
do something like this and then adhere to the rule of zero.

class Circle
{
    private:

        std::string name ;
        // ...

        const std::string* ptr_name() const { return std::addressof(name) ; }

    public:

        // rule of zero

        // ...
};

[keskiverto]

I totally understand that using a regular string would be easier. My requirement is to use name as a pointer.

The 'name' has to be a pointer?
A pointer to a std::string object or a pointer to an array of characters? (Homework can be peculiar.)

Lets assume that name is a pointer to std::string object. We have a problem in

void Circle::operator = (const Circle &rhs)
{
	name = rhs.name; // error
	diameter = rhs.diameter;
	centerX = rhs.centerX;
	centerY = rhs.centerY;
}

Lets be more explicit to help discussion:

void Circle::operator = (const Circle &rhs)
{
	this->name = rhs.name; // error
	this->diameter = rhs.diameter;
	this->centerX = rhs.centerX;
	this->centerY = rhs.centerY;
}

this->name is a pointer.
rhs.name is a pointer.
Each pointer points to an existing std::string object.

You need to copy the value hold in the std::string that the rhs.name points to into the std::string object that the this->name points to.

Can you?

[MisterTams]

keskiverto,

It has to be a pointer to a C++ string so yes, std::string object.
Do I just need to change it to this?
Will that fix it?

void Circle::operator = (const Circle &rhs)
{
	this->name = rhs.name; // error
	this->diameter = rhs.diameter;
	this->centerX = rhs.centerX;
	this->centerY = rhs.centerY;
}

[mbozzi]

If rhs.name is a pointer to a dynamically-allocated std::string then assigning the pointer will result in the same string object being destroyed by both the lhs and rhs, since after assignment both lhs and rhs's name fields point to the same string.
You need to copy the string itself, not merely the pointer to the string.

mbozzi wrote:

Take the second snippet of code in that post and replace SortListClass with Circle.

Circle& operator=(Circle const& rhs) {
  if (std::addressof(rhs) == this) return *this; // careful for self-assignment
  this->~Circle(); 
  new (this) Circle(rhs);
  // Edit: need to return something :)
  return *this
}

http://www.cplusplus.com/forum/beginner/210637/#msg988290

[keskiverto]

You have a pointer. The pointer points to something. That something has a value. How does one reach the value via a pointer?

What do you do in this?

void Circle::setName(string n) {
	*name = n;
}

What does "to dereference a pointer" mean?

[MisterTams]

What does "to dereference a pointer" mean?

This means to use the * operator to get the underlying value of the pointer stored in it's particular memory address (where ever it points to).

[MisterTams]

So I made this change to the overloaded operator =

// operator overloaded as member function
void Circle::operator = (const Circle &rhs)
{
	//copy over values
	delete name;
	name = new string(*rhs.name);
	this->diameter = rhs.diameter;
	this->centerX = rhs.centerX;
	this->centerY = rhs.centerY;
}

Program seems to run normal now.
Is this a valid way of doing this?

[mbozzi]

Not quite. If you perform self-assignment, your program exhibits undefined behavior:

Circle c; 
c = c; // deletes *c.name and then tries to copy it into *c.name 

It also duplicates code which should be in your destructor (freeing the name) and copy constructor (copying name through the pointer), and is exception-unsafe (along with all the other solutions discussed here).

Also you aren't returning anything from the operator, which means that your class fails to support compound assignment

Circle a, b, c; 
a = b = c; // compiler error  

[MisterTams]

Ok so I have another question. If this works and I understand now that I have to delete the memory allocated in (name) for this overloaded operator to work.

// operator overloaded as member function
void Circle::operator = (const Circle &rhs)
{
	//copy over values
	delete name;
	name = new string(*rhs.name);
	this->diameter = rhs.diameter;
	this->centerX = rhs.centerX;
	this->centerY = rhs.centerY;
}

How come a copy constructor doesn't need to delete the memory before allocating more memory?

Such as this copy constructor..

// copy constructor

Circle::Circle(const Circle& obj) 
{
	name = new string(*obj.name);
	diameter = obj.diameter;
	centerX = obj.centerX;
	centerY = obj.centerY;
	numObj++; // increment number of objects created
}

Is it because the object is newly created when the copy constructor is invoked therefore it receives its own memory address for a pointer such as *name?

Ex: Circle c2(c1);

[mbozzi]

How come a copy constructor doesn't need to delete the memory before allocating more memory?

The constructor begins the lifetime of a new object, while the assignment operator overwrites an old object with a copy of another. The assignment operator has to get rid of the old object's stuff, but there is no old object where a constructor is concerned.

Edit:

Is it because the object is newly created when the copy constructor is invoked therefore it receives its own memory address for a pointer such as *name?

Yes. It gets its own name pointer, but it doesn't point to anything -- there's nothing to get rid of.

FWIW, no introductory programming class should even attempt to cover this topic.

[MisterTams]

mbozzi,

Thanks for your help! It all makes a whole lot more sense to me now!