What is the problem with appending arbitrary object to the array?

Question

I have a simple program that process strings from a file into arbitrary class Student (provided below) and appends it to self-written container ArraySequence. When error almost always occurs on marked line (marked with *****).

Class Student:

class Student {
public:
    int year;
    std::string name;
    std::string surname;
    Student(int _year, std::string _name, std::string _surname) {
        this->year = _year;
        this->name = _name;
        this->surname = _surname;
    }
    Student(const Student& s) {
        this->year = s.year;
        this->name = s.name;
        this->surname = s.surname;
    }
    Student& operator=(const Student& s) {
        this->year = s.year;
        this->name = s.name;
        this->surname = s.surname;
        return *this;
    }
    bool operator==(const Student& s) const {
        if (this->year == s.year && this->name == s.name && this->surname == s.surname)
            return true;
        return false;
    }
};

main:

int main() {
    std::ifstream data("filename");
    std::string buffer;
    ArraySequence<Student>* entities = new ArraySequence<Student>;

    getline(data, buffer, '\n');
    while (buffer.size() != 0) {
        std::cout << buffer << std::endl;
        int len = static_cast<int>(buffer.find(" "));
        std::string surname = buffer.substr(0, len);
        buffer = buffer.substr(len + 1);
        len = static_cast<int>(buffer.find(" "));
        std::string name = buffer.substr(0, len);
        int year = std::stoi(buffer.substr(len + 1));
        Student b(year, name, surname);
        entities->append(b); *****
        getline(data, buffer, '\n');
    }

    std::cout << count << std::endl;

Append:

template <typename T>
void ArraySequence<T>::append(const T& item) {
    ISequence<T>::length++;
    if (!(ISequence<T>::length - 1)) {
        data = (T*)malloc(sizeof(T));
    }
    else {
        data = (T*)realloc(data, sizeof(T) * ISequence<T>::length);
    }
    *(data + ISequence<T>::length - 1) = item;
}

Errors: Thread 1: EXC_BAD_ACCESS (code=EXC_I386_GPFLT); Thread 1: EXC_BAD_ACCESS (code=1, address=0x3);

Sometimes it would just work, but other times it crashes with errors.

I'm using Xcode 11.1

In Thread 1 it says that problem with operator= of Student on the line c++ this->surname = s.surname;

What can I do in a situation like this? Any ideas?

The segfault occurs when calling a method of a class that's not even shown in your question, `ArraySequence`. But the problem can be anywhere in your program. Just because a program crashes at a particular place doesn't mean that's where the bug is. C++ does not work this way. You need to read stackoverflow.com's requirements for a [mre], and [ask]. Unless you show sufficient information that will allow anyone to cut/paste, compile, and reproduce your problem themselves, it is unlikely that anyone will be able to help you. — Sam Varshavchik, Nov 17 '19 at 19:18
What is `Array`? And if its purpose is to be a dynamic array, why aren't you using `std::vector` for this purpose? Also `ArraySequence* entities = new ArraySequence;` -- in C++, there is no need for `new` to create objects. Instead: `ArraySequence entities;` — PaulMcKenzie, Nov 17 '19 at 19:19
@PaulMcKenzie because using self-written Array class is a part of this task. — l.marder, Nov 17 '19 at 19:22
First, that is not the entire `ArraySequence` class. Second, `malloc` and `realloc`? In a C++ program? Your program is broken from the start, since you are not creating objects with `malloc`, and your `Student` class has `std::string` members. — PaulMcKenzie, Nov 17 '19 at 19:23
@PaulMcKenzie the entire ```ArraySequence``` is too huge to put here. Yes, ```malloc``` and ```realloc``` because I find them more convenient (possibly mistakenly) than using ```new``` for this purpose. — l.marder, Nov 17 '19 at 19:30
Your code is wrong, period, full stop. Your `Student` class contains `std::string` members, meaning that it needs to be *constructed*, not just memory allocated. C++ is not just `C` with a few syntax differences. When you create non-trivial types, they need to be constructed, else the behavior is undefined. [See this](https://stackoverflow.com/questions/2995099/malloc-and-constructors) — PaulMcKenzie, Nov 17 '19 at 19:32
@PaulMcKenzie I understand. Is there any way to reallocate memory dynamically in C++ as ```realloc``` does it? — l.marder, Nov 17 '19 at 19:34
You will just have to use the `new[]` allocation, copy, `delete[]` deallocation steps. Also, a better implementation of your `ArraySequence` would overallocate (maybe double the current amount), so that a simple `append` wouldn't automatically invoke a reallocation (a `capacity` member would be introduced).. Imagine if somewone were to call `append` in a loop of 100 or 1000 items? You would do the `realloc` 100 or 1000 times? — PaulMcKenzie, Nov 17 '19 at 19:39
@PaulMcKenzie in this case, ```Array``` becomes much slower? Or I'm thinking wrong? I get your idea about ```capacity```, thank you! — l.marder, Nov 17 '19 at 19:43
That's why the strategy of overallocating is done, so that you're not calling allocation functions every single time. So it is not necessarily slower or faster, just less calls to the heap allocation functions. — PaulMcKenzie, Nov 17 '19 at 19:44
@PaulMcKenzie and the last question: Isn't overallocating leads to a loss of space? — l.marder, Nov 17 '19 at 19:47
It is a tradeoff between space and speed. Calling the allocator each and every time an item is appended is deemed more costly than giving up some bytes of extra space. — PaulMcKenzie, Nov 17 '19 at 19:52

score 1 · Accepted Answer · edited Nov 18 '19 at 00:54

T consists of a non-trivial type (Student contains std::string, plus non-trivial copy/ assignment and destructor), using malloc and realloc will not work correctly.

malloc and realloc do not create viable objects. They only allocate memory. Your program likely crashes due to these invalid Student objects.

The easiest solution is to rewrite ArraySequence<T>::append to only use new[] and delete[], not malloc and realloc.

Note: malloc can be used when using placement-new, but you were not using malloc for this purpose.

What is the problem with appending arbitrary object to the array?

1 Answers1

Linked