[RESOLVED] Array of objects - member variable in free store error

[san1]

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

class Employee{
protected:
char* name;
public:
Employee(){ // Default constructor used to declare arrays
name="empty";
cout << "Default constructor called for " << name << endl;
}
Employee(char* name_1){
name=new char[strlen(name_1)+1];
name=name_1;
cout << "Explicit constructor called for " << name << endl;
}
~Employee(){
delete [] name;
cout << "Destructor called for " << name << endl;
}
char* show(){return name;}
};
int main(){
cout << "----------------------" << endl;
Employee Executive[2];
Executive[0]=Employee("Alex");
Executive[1]=Employee("Mery");
cout << "your user is " << Executive[0].show() << endl;
cout << "your user is " << Executive[1].show() << endl;
return 0;
}
------------------------------------------------------------

array is created and destroyed as soon as it created. Why ?
There are other problems also what's wrong with this code ?

[VictorN]

array is created and destroyed as soon as it created. Why ?
There are other problems also what's wrong with this code ?

The first thing you did wrong is you did not formatted your code properly, so it is very hard to read/understand it.

As for you problem: did you try to debug your code, particularly this line:

Executive[0]=Employee("Alex");

Just set a break point, press F5, then F11... (in MS IDE)

[Paul McKenzie]

First, please use code tags when posting code. The code you posted is not formatted and very hard to read.

There are a few things wrong with your code, and a few things you need to learn about C++ and object creation:

1)

#include<iostream.h>

The correct header is <iostream>, not <iostream.h>. There is no such header as <iostream.h> in ANSI C++.

After including <iostream>, then I/O classes are in the std namespace.

I can bomb your code out with just one line main()

int main()
{
   Employee e;
}

Do you know why it bombs out? Because you are attempting to delete[] the name, and the name was not dynamically allocated in the destructor of Employee.

So before even explaining what's wrong with your code, if a one-line program doesn't work, it would make sense to fix that before even going on any further.

Your Employee class not only has that error, it lacks a copy constructor and assignment operator that will handle the pointer to dynamically allocated memory. There are literally hundreds of threads on this topic, and should have been discussed in the C++ book you're using.

Having said this, the easy way to not have these issues is to use std::string instead of char pointers and new[]/delete[]. There is no reason to be using new[]/delete[] in any C++ program, except for the case if you're creating your own memory allocator. For string data, std::string makes everything work correctly.

#include <string>
class Employee
{
    protected:
    std::string name;

    public:
        Employee(const std::string& sName = "empty") : name(sName) { }
};

That entire code is all you need, and everything works correctly.

Regards,

Paul McKenzie

[san1]

Sorry to everyone guys for bad formatted code. I will format code properly when I post something in future. I compiled this code in Turbo C just to be quick.
Anyway thanks for your quick response.

[PredicateNormative]

Please use [code][/code] tags around code when posting, else you lose the formatting and it becomes difficult to read.

There are a number of things wrong with your program. So what I will do is re-print your code with code tags and put some numbers against the items that have issues

#include<iostream.h> //1
#include<string.h>  //2

class Employee
{
protected:
  char* name;
public:
  Employee()
  { // Default constructor used to declare arrays
    name="empty"; //3
    cout << "Default constructor called for " << name << endl;
  }

  Employee(char* name_1)
  {
    name=new char[strlen(name_1)+1];
    name=name_1; //4
    cout << "Explicit constructor called for " << name << endl;
  }
  
  ~Employee()
  {
    delete [] name;
    cout << "Destructor called for " << name << endl;
  }
  
  char* show(){return name;}
};

int main()
{
  cout << "----------------------" << endl;
  Employee Executive[2];
  Executive[0]=Employee("Alex");
  Executive[1]=Employee("Mery");
  cout << "your user is " << Executive[0].show() << endl;
  cout << "your user is " << Executive[1].show() << endl;
  return 0;
}

Here are my comments

1) iostream.h is not a standard C++ header and should not be used. The correct standard header is iostream, therefore the correct include declaration is:

 #include <iostream>

2) string.h is not a C++ header. The correct header is

 #include <cstring>

3) When you write the following

 name = "empty"

you are assigning a string literal to a pointer. A string literal has static storage duration, and attempting to call the delete[] expression on a pointer pointing to a string literal will yield undefined behaviour. With the exception of setting the pointer to null, if you are going to use the pointer for dynamic allocation and deallocation then you should not use it for pointing to anything other than memory that you allocate and deallocate with that pointer. The point is, be consistent, otherwise you will get yourself in trouble. So what you can do is allocate some memory to name using new[] and then assign "empty" to it.

4) The following line does not do what you think:

name=name_1;

What it is doing is assigning the name_1 pointer to the name pointer, what it is not doing is assigning the contents of memory pointed to by the name_1 pointer, to the contents of the memory pointed to by the name pointer. In order to do that, you need to use memcpy. Passing a null pointer to memcpy yields undefined behaviour, so you will first need to check that name_1 is not null before attempting a copy. So, in short, the above line should be changed to something along the lines of:

if(name_1 != 0)
{
  memcpy(name, name_1, strlen(name_1));
}
else
{
  //Do something in here
}

Finally, you could save your self a lot of trouble just by using the standard string class:

#include<iostream>
#include<string>

class Employee
{
protected:
  std::string name;
public:
  Employee()
  { // Default constructor used to declare arrays
    name="empty";
    cout << "Default constructor called for " << name << endl;
  }

  Employee(char* name_1)
  {
    name=name_1; 
    cout << "Explicit constructor called for " << name << endl;
  }
    
  const char* show() const {return name.c_str();}
};

int main()
{
  cout << "----------------------" << endl;
  Employee Executive[2];
  Executive[0]=Employee("Alex");
  Executive[1]=Employee("Mery");
  cout << "your user is " << Executive[0].show() << endl;
  cout << "your user is " << Executive[1].show() << endl;
  return 0;
}

EDIT:
Just for information, I posted the above having not seen Paul response - although I guess that might be obvious .

[Paul McKenzie]

Sorry to everyone guys for bad formatted code. I will format code properly when I post something in future. I compiled this code in Turbo C just to be quick.

That compiler is over 20 years old!. You shouldn't even consider compiling anything using that dinosaur. It is not ANSI compliant, and any advice we give you may not even work or be implemented with such an old compiler.

Regards,

Paul McKenzie

[Eri523]

Just to mention...

  memcpy(name, name_1, strlen(name_1));

That's the same as:

  strcpy(name, name_1);

[san1]

I understand the error no. 1, 2 and 4.
But error no. 3 is somewhat confusing to me.
I have some questions
1)
Why array elements Executive[0] and Executive[1] are destroyed as soon as they are created. I have learned that objects gets

destroyed when goes out of scope or memory which is allocated in free store, need to be free .
2)
I have dynamically allocated only array elements Executive[0] and Executive[1] in free store, not Executive[]. So my

assumption is, only Executive[0] and Executive[1] should call destructor not the Executive[].

If you think I should do some homework before asking stupid questions like this then please suggest me any resources on web.
Or if you correct the code I might understand something.

[Paul McKenzie]

I understand the error no. 1, 2 and 4.
But error no. 3 is somewhat confusing to me.
I have some questions
1)
Why array elements Executive[0] and Executive[1] are destroyed as soon as they are created.

You are doing many things in two lines of code.

Employee Executive[2];

This constructs two Employee objects. The default constructor is called for each object.

Executive[0]=Employee("Alex");

You are constructing a temporary object, Employee("Alex"). Then, this temporary is assigned to element 0 of the array, calling the assignment operator to do this (the "=") . Then the temporary is destroyed (the destructor of the item in the right side of the equal sign is called).

In other words, if you break the second-line down:

1)  Employee("Alex");  // creates temporary Employee, let's call it T.

2)  Executive[0] =  T; // calls assignment operator to assign temporary T to Executive[0].  The "=" is responsible for this magic.

3) ~T(); //  called by the compiler.  Temporary goes away, so destructor for Employee("Alex") is called

So you are not just doing simple assignment, as if you're working with arrays of ints or doubles. Construction, assignment, constructing temparies, destruction, are done on just those two lines. That's why it is important to write a user-defined copy constructor and assignment operator. You don't know when or where it will be called, and the code above proves my point.

But you can forget all of this if you just used std::string.

Regards,

Paul McKenzie

[san1]

Thank's Paul, Predicate and other guys.
Paul, you had already mentioned that I need assignment operator and copy constructor, but I simply overlooked assignment operator and thought that both are not necessary here.

Thanks everyone guys.

[PredicateNormative]

Just to mention...



That's the same as:

  strcpy(name, name_1);

Yeah, I kind of forgot about strcpy.