0

So my goal is to create many objects of the same class without copying one object many times. I think i found a way by making an array of pointers that point to a certain object but i get a segmentation fault.

Unfortunately in this project I'm trying to do, I'm not using any other libraries except <cstring> (I have to add here that the constructor of the class has some parameters as input).

Thoughts?

Here is the code:

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

int main(void)
{
    int i, floor, classroom; //the floor of the school and the classroom the student is heading to
    char *stname;
    typedef Student *stptr;
    stptr *students = new stptr[15];
    for (i = 0; i < 15; i++)
    {
        cin >> stname;
        cin >> floor;
        cin >> classroom;
        students[i] = new Student(stname, floor, classroom);
    }
}

...and the code for the class:

class Student
{
private:
    char *name;
    int no_floor;
    int no_classroom;
public:
    Student(const char *nam, int no_fl, int no_cla) //constructor
    {
        name = new char[strlen(nam + 1)];
        strcpy(name, nam);
        no_floor = no_fl;
        no_classroom = no_cla;
        cout << "A new student has been created! with name " << name << " heading to floor: " << no_floor << " class: " << no_classroom << endl;
    };

    ~Student() //destructor
    {
        cout << "A Student to be destroyed! with name " << name << " is at floor: " << no_floor << " class: " << no_classroom;
        delete[] name;
    };
};
anastaciu
  • 23,467
  • 7
  • 28
  • 53
  • 3
    `strlen(nam+1)` ? - maybe you meant: `strlen(nam) + 1` ? – marcinj Nov 09 '20 at 11:41
  • 2
    If this is C++, then read the Isocpp Core Guidelines and get rid of the naked new and c style string pointers. Use standard types like `vector` and `string`. Regarding your segfault: `new char[strlen(nam+1)]` is wrong, you want `new char[strlen(nam)+1]`. – Werner Henze Nov 09 '20 at 11:43
  • @marcinj you're right thanks! – ScatterBrainer Nov 09 '20 at 11:45
  • 1
    `stname` is an uninitialised pointer, use `std::string` instead – Alan Birtles Nov 09 '20 at 11:49
  • @WernerHenze thanks for the notice on new char[strlen(nam)+1] ! Unfortunately it didn't remove the segfault – ScatterBrainer Nov 09 '20 at 11:50
  • 1
    Don't use manual memory allocation at all. Use a `std::vector` and remove all the `new`ing of students from your code. – super Nov 09 '20 at 12:01
  • 1
    Additionally, you need to supply a [mcve]. You are missing the `Student` definition and a main function. Make it minimal, meaning only the code you need to reproduce the error. – super Nov 09 '20 at 12:02
  • @super will do immediately – ScatterBrainer Nov 09 '20 at 12:14
  • if you know the length of the array at compiletime, you might look into std::array. you can use a raw array with a smart pointer,too. – Raildex Nov 09 '20 at 16:10

4 Answers4

3

I would advise against this kind of pointer obfuscation:

Student** students = new Student*[15];

Is easier to understand IMO.

You could use the tools privided by C++ to automatically handle strings arrays of objects:

std::string for strings;

std::vector for the array.

Object pointers can be useful in the context of polymorphism, but even then it's better to use smart pointers.

As pointed out by marcinj in the comments the name declaration is also in need of fixing.

And as also pointed out by Alan Birtles you are using stname uninitialized, this is the likely culprit of the segfault.

To fix your code, using the tools you are currently using, you shoud do something like the code below, note that aside from the fixes, the rule of three must be respected:

#include <iostream>
#include <cstring>
class Student {
    char *name;
    int no_floor;
    int no_classroom;
public:
    Student(const char *nam, int no_fl, int no_cla){//constructor  
        name = new char[strlen(nam) + 1];
        strcpy(name, nam);
        no_floor = no_fl;
        no_classroom = no_cla;       
        std::cout << "A new student has been created! with name " << name << " heading to floor: " << no_floor << " class: " << no_classroom << std::endl;
    };
    ~Student(){//destructor    
      std::cout << "A Student to be destroyed! with name " << name << " is at floor: " << no_floor << " class: " << no_classroom;
      delete [] name;
    };    
    Student (const Student& student) { //copy constructor
        this->name = nullptr;
        *this = student;
    } 
    Student& operator = (const Student& student){ //assignment operator
        if(this == &student){
            return *this;
        }
        delete [] this->name;
        this->name = new char[strlen(student.name) + 1];
        strcpy(this->name, student.name);
        this->no_classroom = student.no_classroom;
        this->no_floor = student.no_floor;        
        return *this;
    }
};
int main(){

    int i, floor, classroom;
    char *stname = new char[100];
    typedef Student *stptr;
    stptr *students = new stptr[15];

    for (i = 0; i < 15; i++){

        std::cin >> stname;
        std::cin >> floor;
        std::cin >> classroom;
        students[i] = new Student(stname, floor, classroom);
    }
    delete [] stname;

    for (i = 0; i < 15; i++){
        delete students[i];
    }
    //or for the deletion in inverse order
    //for (i = 2; i >= 0; i--){ 
    //    delete students[i];
    //}
    delete [] students;

}
anastaciu
  • 23,467
  • 7
  • 28
  • 53
  • Comments are not for extended discussion; this conversation has been [moved to chat](https://chat.stackoverflow.com/rooms/224344/discussion-on-answer-by-anastaciu-trying-to-create-many-objects-of-the-same-clas). – Samuel Liew Nov 10 '20 at 01:48
1

There is no need to use pointers in this code, using an array of objects rather than pointers and std::string rather than char* makes your code much simpler and safer:

#include <iostream>
#include <string>

class Student
{
private:
    std::string name;
    int no_floor;
    int no_classroom;
public:
    Student() = default;

    Student(const std::string& nam, int no_fl, int no_cla) //constructor
    : name(nam), no_floor(no_fl), no_classroom(no_cla)
    {
        std::cout << "A new student has been created! with name " << name << " heading to floor: " << no_floor << " class: " << no_classroom << std::endl;
    };

    ~Student() //destructor
    {
        std::cout << "A Student to be destroyed! with name " << name << " is at floor: " << no_floor << " class: " << no_classroom;
    };
};

int main(void)
{
    int i, floor, classroom; //the floor of the school and the classroom the student is heading to
    std::string stname;
    Student students[15];
    for (i = 0; i < 15; i++)
    {
        std::cin >> stname;
        std::cin >> floor;
        std::cin >> classroom;
        students[i] = Student(stname, floor, classroom);
    }
}

There are a number of problems in your original code.

  1. stname is uninitialised, therefore reading into it will result in undefined behaviour. You need to initialise it with an array e.g. char *stname = new char[100], you'll have to remember to delete it at the end of your program to avoid a memory leak. Also note that std::cin >> stname is unsafe as it doesn't know the length of the array so if you read more characters than will fit it will result in undefined behaviour. Alternatively you could just use a fixed length array which at least solves the problem of having to remember to call delete: char stname[100]
  2. You never delete the elements of students or students itself resulting in a memory leak.
  3. strlen(nam + 1) returns the length of the string starting from the second character what you actually want is strlen(nam) + 1 which returns the length of the string plus a null terminator.
  4. It isn't causing you a problem (yet) but you need to implement the rule of three and add a copy constructor and assignment operator

All 4 of these problems disappear when you aren't using raw pointers...

If you really can't use std::string the best approach is probably to make your own string class and use that instead so that all the raw pointer crud is at least constrained into a single place. For example something like this:

class String
{
public:
    String()
    :str(nullptr), length(0), capacity(0)
    {
    }

    String(const char* s)
    :String()
    {
        assign(s);
    }

    String(const String& other)
    :String()
    {
        assign(other.str, other.length);
    }

    String& operator = (const String& other)
    {
        assign(other.str, other.length);
        return *this;
    }

    ~String()
    {
        delete[] str;
    }

    void reserve(size_t new_capacity)
    {
        // note doens't copy existing data in str to new buffer
        delete str;
        str = new char[new_capacity + 1];
        capacity = new_capacity;
    }

    friend std::ostream& operator <<(std::ostream& os, const String& str)
    {
        if (str.length)
        {
            os.write(str.str, str.length);
        }
        return os;
    }

    friend std::istream& operator >>(std::istream& is, String& str)
    {
        // note can only read up to 100 characters
        str.reserve(100);
        str.length = 0;
        std::istream::sentry sentry(is);
        if (sentry)
        {
            while (str.length < str.capacity)
            {
                auto ch = is.rdbuf()->sgetc();
                if (!std::istream::traits_type::not_eof(ch))
                {
                    is.setstate(std::ios_base::eofbit);
                    break;
                }
                if (std::isspace(ch))
                {
                    break;
                }
                str.str[str.length++] = ch;
                is.rdbuf()->sbumpc();
            }
        }
        str.str[str.length] = '\0';
        return is;
    }

private:
    void assign(const char* s)
    {
        size_t len = strlen(s);
        assign(s, len);
    }
        
    void assign(const char* s, size_t len)
    {
        if (len > 0)
        {
            if (len > capacity)
            {
                reserve(len);
            }
            strcpy(str, s);
        }
        length = len;
    }

    char* str;
    size_t length;
    size_t capacity;
};

Yes it's a lot of code but it's the bare minimum you need for your code to be correct if your teachers have an allergy to std::string.

Alan Birtles
  • 32,622
  • 4
  • 31
  • 60
  • Thank you very much for your answer and for taking the time to explain! I'am allowed to use the library thankfully. – ScatterBrainer Nov 09 '20 at 18:01
  • Though i do detect some problems in your code. As soon as the student gets created the destructor gets called immediately plus the destructors of the objects get called again at the end. So that makes 2 destructor calls – ScatterBrainer Nov 09 '20 at 18:14
  • @ScatterBrainer yes, a temporary object is created and gets deleted after it is copied into the array, that's how C++ works, if your teachers aren't even teaching that I suggest finding some good c++ books – Alan Birtles Nov 09 '20 at 18:15
  • ok thanks for the notice! I think i'm going with @/anastaciu 's answer though beacause that double destructor message print might look a little off at first. – ScatterBrainer Nov 09 '20 at 18:19
  • i do have to credit this answer though for deleting the objects in the right order unlike the other one. If anyone has any suggestions on how to "hide" the destructor message of the temporary objects i would like to hear them! – ScatterBrainer Nov 09 '20 at 18:49
  • @ScatterBrainer, This is deffinitely a good answer, I did want to create a custom class using rule of three just for kicks, but this is simpler and aguably more effective, still, I don't understand what you mean by *"deleting the objects in the right order"*, the order is the same in both methods, and the extra print is there because the object is created and then copied, aside form the extra verbosity because you print things in your destructor, there is nothing wrong with it. – anastaciu Nov 09 '20 at 18:55
  • @anastaciu Say we create object a,b and c.Constructor a gets called, constructor b gets called, constructor c gets called(i think we can all agree on that).Your solution (which once again i'm very thankful for!) calls destructor a, destructor b then destructor c. The above solution calls destructor c, destructor b, destructor a. – ScatterBrainer Nov 09 '20 at 19:00
  • 1
    @ScatterBrainer, oh I see, that's just a matter of inverting the delete for loop, *i.e.* `for (i = 2; i >= 0; i--){ delete students[i];}` but again, this is a good answer and you do well using it. – anastaciu Nov 09 '20 at 19:05
0

Although I agree with @anastaciu 's comment, I would also suggest to avoid the usage of raw pointers since if you are new to C/C++ you might encounter a lot of issues such as dangling pointers, wild pointers and memory leaks. You shall either avoid them unless necessary(such as memory mapped IO or unsafe casts because you cannot think of another way) or, use a built container to place your objects (you can read more about them here).

-1

ok i found a solution to the double destruction prints that came up in the answer by @Alan Birtles. You can allocate memory a diffrent way without making a copy of the object then destroying it. Here's the code:

int i,floor,classroom;
   string stname;
   Student* students[5];
   for(i=0; i<5; i++)
   {
      cin >> stname;
      cin >> floor;
      cin >> classroom;
      students[i] = new Student(stname, floor, classroom);
   }
   for(i=0; i<5; i++)
   {
     delete students[i];
   }

..and there you have it!