Malloc error - Something to do with the deconstructor

[CoderParzival]

Design a class called Employee. Separate the declaration and methods definition file. The class
should store the following employee information:
name: employee's name which is a pointer points to a C-string
eid: employee ID, type is string
salary: type is double
Define the following member functions:
Constructors: default constructor, parameterized constructor with name, eid, and salary as
parameters, and a copy constructor.
Destructor
Mutator functions: setName(char *name) , setEid(string eid), setSalary(double s)
Accessor functions: getName(), getEid(), getSalary()
Overload assignment operator: overload the assignment operator '=' for deep copy, and make
sure it allows multiple assignment statements, for example e1=e2=e3
Use Lab08.cpp to test your class.

Here is Lab08.cpp

  
#include "Employee.h"
#include<iostream>
#include<string>

using namespace std;

int main()
{
    Employee e1("John","e222", 60000), e2(e1), e3, e4;
    
    e3 = e4 = e2;
    
    e2.setName("Michael");
    e2.setSalary(75000);
    e3.setName("Aaron");
    e3.setSalary(63000);
    e4.setName("Peter");
    
    
    cout << "\nName: " << e1.getName() << "\nID: " << e1.getID() << "\nSalary: " << e1.getSalary() << endl;
    cout << "\nName: " << e2.getName() << "\nID: " << e2.getID() << "\nSalary: " << e2.getSalary() << endl;
    cout << "\nName: " << e3.getName() << "\nID: " << e3.getID() << "\nSalary: " << e3.getSalary() << endl;
    cout << "\nName: " << e4.getName() << "\nID: " << e4.getID() << "\nSalary: " << e4.getSalary() << endl;
    
    return 0;
}

I'm having an issue saying this after the program runs it's reading this

lab8(25397,0x100396340) malloc: *** error for object 0x100002ed0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
(lldb)

I'm not sure how to get rid of it, but I believe it has something to do with my deconstructor.

//
//  Employee.h
//  Test
//
//  Created by Jayme Woodfill on 4/2/18.
//  Copyright © 2018 Jayme Woodfill. All rights reserved.
//

#ifndef Employee_h
#define Employee_h

#include <iostream>
#include <string>
using namespace std;

class Employee
{
private:
    char *name; //Points to a C-string
    string eid; //employees id
    double salary; //salary
    int SIZE = 40;
public:
    
    //Default Constructor
    Employee(){
        eid = "0";
        salary = 0.0;
        //Don't know what to set here
    }
    //Parameteized Constructor
    Employee(char *n, string id, double sal)
    {
        eid = id;
        salary = sal;
        name = new char[SIZE];
        for(int i = 0; i < SIZE; i++)
            name[i] = n[i];
    }
    //Copy Constructor
    Employee(const Employee &obj)
    {
        eid = obj.eid;
        salary = obj.salary;
        name = new char[SIZE];
        for(int i = 0; i < SIZE; i++)
            name[i] = obj.name[i];
    }
    //Destructor
    ~Employee()
    { delete [] name;}
    
    //Sets the name
    void setName(char *n)
    {
        name = n;
    }
    //Get's the name
    string getName() const 
    {
        return name;
    }
    
    //Set the employees id number
    void setEid(string id)
    { eid = id; }
    
    //Get the employees id number
    string getID () const
    {return eid;}
    
    //Set the salary
    void setSalary(double s)
    { salary = s;}
    
    //Get salary
    double getSalary() const
    { return salary;}
    
    //Get a name
    char getName(int index) const
    { return name[index];}
    
    //Overloaded = operator
    const Employee operator=(const Employee &right)
    {
        delete [] name;
        eid = right.eid;
        salary = right.salary;
        name = new char[SIZE];
        for(int i = 0; i < SIZE; i++)
            name[i] = right.name[i];
        return *this;
    }
};

#endif /* Employee_h */ 

[Ganado]

In your default constructor, try setting name = nullptr;

Also, I didn't run your code, but note that the overloaded = operator should return a non-const reference to *this, in most cases. You have it returning a copy.

[CoderParzival]

I changed the overloaded = operator to non-const and nothing changed. I believe it has something to do with my deconstructor because I am receiving a warning which reads "Thread 1: signal SIGABRT" after the program runs.

[Ganado]

The problem is that you are trying to call delete [] on what is currently a junk value (name), in case that isn't clear. So yes, the immediate problem is the destructor, but the real problem that it shouldn't happen that the destructor is trying to delete junk data to begin with.

You said you updated the =operator, but did you set name to nullptr in your constructor?
You need to do that, in addition to my edits below. If nullptr doesn't work, do name = NULL;

EDIT: I found your real problem. Give me a minute to type it.

Wait, that multiplied. You have 2 problems.

1.

Your problem is this

    //Sets the name
    void setName(char *n)
    {
        name = n;
    }

You call it, for example, as e2.setName("Michael");

The problem here is that you are setting the pointer to now point to static, non-heap data. You cannot use delete or delete[] on this type of data. You can only delete what you new.
You also have a memory leak because you are not destroying what is already in name.

Try changing it to something like this:

#include <cstring> // include this at the top of your file

    //Sets the name
    void setName(const char *n) // Note the const! Your compiler is outdated; it should warn for not using const here
    {
        delete [] name;
        int size = strlen(n) + 1;
        name = new char[size];
        for (int i = 0; i < size; i++)
        {
            name[i] = n[i];
        }
    }

2.

Your second problem is that you are accessing out-of-bounds data in both of your constructors.

        for(int i = 0; i < SIZE; i++)
            name[i] = n[i];

you are iterating from n[0] to n[SIZE-1], but n is less than 40 characters long.
Only iterate up to strlen(n) + 1, not SIZE.

Note: The +1 is to account for the null-terminator for a c-string.

[CoderParzival]

This makes complete sense. The only problem now is when I run it the code reads "Thread 1: EXC_BAD_ACCESS (code=1, address=0x0)" When dealing with this line

name = new char[strlen(name) + 1];

[CoderParzival]

Nevermind i fixed the code by changing it to this:

The original:

name = new char[strlen(name) + 1];
        for(int i = 0; i < strlen(name) + 1; i++)
            name[i] = obj.name[i];

New and improved:

name = new char[strlen(obj.name) + 1];
        for(int i = 0; i < strlen(obj.name) + 1; i++)
            name[i] = obj.name[i];

[TheIdeasMan]

Hi,

If only you would use a std::string instead of a dynamically allocated char array, then all your memory management problems will go away. std::string dynamically allocates it's data just like all the other STL things.

One should never need to use new in modern c++ , unless one is writing a library.

Some common and bad reasons why people use new :

* It's "cool" to use dynamic memory, not realizing that the STL already does it for you safely ;
* Just to obtain a pointer. Bad on 2 fronts there, shouldn't really need to use pointers either, unless it's a smart pointer ;
* they were taught to do that by their behind the times teacher ;
* they are still in a C mindset, and feel the need to manage everything themselves
* they are still in a C mindset, and think that new is a C++ version of malloc, but don't relize the danger of using new

The biggest problem with new is if something throws an exception, the destructor is never reached. This is why using the STL or smart pointers is the way to go.

[CoderParzival]

Hi TheIdeasMan,

Your response was kind of rude :/ Obviously I'm using a Char array because it was specified in the question if you had read it.

Some common and bad reasons why people don't read:

*They think they can just skim over the information and know exactly what they're doing;
*They're too "cool" to read because they know all;
*They were taught by some "behind the times" person their ideas are more important than actually reading other people's questions and answering them properly;
* They're still in a Self Centered Mindset (SCM), and feel the need to know and manage everything themselves;
* They're still in a Self Centered Mindset (SCM), and think skimming the question is the same as reading, but don't realize the danger of just skimming material;

The biggest problem with not reading is if you miss something simple in the question, you come off as arrogant and a know it all. This is why reading is the way to go.

[helios]

Everyone here is donating their time to try and help you. Regardless of how you feel about the tone of his post, TheIdeasMan was trying to be helpful. If you feel he was rude to you you should just say that. You don't need to be rude in response.

[Cubbi]

jjwood15 wrote:

Obviously I'm using a Char array because it was specified in the question

He's simply pointing out one of the reasons (there are others) why whoever wrote that "Lab 8" doesn't know C++ or how to teach it. Don't take it personal.

[CoderParzival]

I'm not upset because he suggested the string, I'm upset about the way he went about it and his wording of "helping" it's not helping if you don't actually help someone out and just tear them down and call what they're doing stupid of "behind the times". That's just being rude. While my response might have been rude it was literally the exact same thing he said, just replaced the words.

[helios]

Don't abuse the reporting function. This is not what it's for.

just tear them down and call what they're doing stupid of "behind the times"

He wasn't tearing you down, he was just pointing out some common (and bad) reasons why people often use naked pointers and manual memory management. And in your case he happens to be right. Your instructor probably learned C++ some time in the 90s and never updated their knowledge.
Regardless of how you feel about it, what you're doing is behind the times. No one writes C++ like this anymore, because of how brittle this kind of code is.
I'm sorry if this hurts your feelings, but it's just the way it is.

While my response might have been rude it was literally the exact same thing he said, just replaced the words.

Your response isn't rude because of what it says, but because it's sarcastic. TheIdeasMan's post can be interpreted as not being rude (I personally don't think it is) even if you think it is, but yours can't.

[CoderParzival]

okayyyyy