Program fails when I call a method from constant reference

Question

So, I have a token class: Token.h

class Token {
    std::string name; // token name
    int frequency;//frequency
    Vector lines;//lines where the token is present

public:
    //explanations for the methods in the Token.cpp
    Token(std::string tokenname, int linenumber);
    virtual ~Token();
    const Vector getLines() const;
};
#endif /* TOKEN_H_ */

Token cpp

Token::Token(string tokenname, int linenumber) {
    // TODO Auto-generated constructor stub
    name = tokenname;
    frequency=1;
    lines.push_back(linenumber);
}
Token::~Token() {
    // TODO Auto-generated destructor stub
}
std::string Token::getName() const{
    return name;
}
int Token::getFrequency() const{
    return frequency;
}
const Vector Token::getLines() const{
    const Vector vec = lines;
    return lines;
}

The program fails, when I pass it to the insert method of list class

class List {
private:
    class Node {
    public:
        Token data;
        Node* next;
        Node(const Token &dataItem, Node* nextptr);
        ~Node();

    };
    Node* first;
    int length;
public:
    List();
    virtual ~List();
    void insert(const Token &t);
};

List.cpp:

List::Node::Node(const Token &dataItem, Node* nextptr): data(dataItem), next(nextptr){
}
List::Node::~Node(){
cout<<"dead"<<endl;
}

List::List() {
    // TODO Auto-generated constructor stub
    length = 0;
    first = nullptr;
}

List::~List() {
    // TODO Auto-generated destructor stub
    Node* temp = first;
    Node* newtmp;
               while(temp->next != nullptr){
                   newtmp = temp->next;
                   delete temp;
                   temp = newtmp;
               }
}
const int List::size(){
    return length;
}

void List::insert (const Token &t){

        Vector dammit = t.getLines();

}

I found out which line in the insert does it(Vector dammit = t.getLines()), so I leave it like that. It gives me this error message:

double free or corruption (fasttop): 0x0000000000c34040 ***

And here something from main file if you want to run:

int main() {
//  cout<<"tokens are here"<<endl;
//
    Token hit("aca", 1);
    Token hit2("ui", 2);
    Token hit1("111", 3);
    List list;
    list.insert(hit);
    list.insert(hit2);
    list.insert(hit1);
}

Vector class:

class Vector {

int* store;
int capacity;
int next_index;
public:
    Vector();
    Vector(int initial_size);
    Vector(const Vector &v);
    virtual ~Vector();
    void push_back(int item);
    int pop_back();
    const int size() const;
    void resize();
    void operator =(const Vector &v);
    int& operator[] (int k);
    const int& operator[] (int k) const;
    friend std::ostream& operator<<(std::ostream& os, const Vector& v);

};
Vector::Vector() {
    // TODO Auto-generated constructor stub
    store = new int [1];
    capacity = 1;
    next_index = 0;
}

Vector::Vector(int initial_size){
    store = new int [initial_size];
    capacity = initial_size;
    next_index = 0;
}

Vector::Vector(const Vector &v){
    store = v.store;
    capacity = v.capacity;
    next_index = v.next_index;
}

Vector::~Vector() {
    // TODO Auto-generated destructor stub
    delete[] store;
}
void Vector::resize(){
    std::cout<<"in resize"<<std::endl;
std::cout<<capacity<<std::endl;
    int length = capacity;
    capacity+=100;
    int* tempArray;
    tempArray = new int[capacity];
    for (int i=0; i<length; i++){
        tempArray[i] = store[i];
    }
    if (length>1)
    delete[] store;
    std::cout<<"finish re4size"<<std::endl;
    store = tempArray;


}

void Vector::push_back(int item){
    if(next_index >= capacity)
        this->resize();
    store[next_index] =item;
    next_index++;
}

int Vector::pop_back(){
    next_index = next_index-1;
    int last = store[next_index];
    return last;
}

void Vector::operator =(const Vector &v){
    //delete[] store;
    store = v.store;
    capacity = v.capacity;
    next_index = v.next_index;
}
const int Vector::size() const{
    return next_index-1;
}
int& Vector::operator[] (int k){
    //assert((k<next_index)&(k>=0));
    return store[k];
}
const int& Vector::operator[] (int k) const{
    //assert((k<next_index)&(k>=0));
    return store[k];
}
ostream& operator<<(ostream& os, const Vector& v)
{
    for(int i=0; i<=v.size(); i++){
       os <<  v[i]<< ' ';
    }
    return os;
}

Answer 1

In

Vector::Vector(const Vector &v){
    store = v.store;
    capacity = v.capacity;
    next_index = v.next_index;
}

You now have two vectors pointing to the same int* store;

In

void Vector::operator =(const Vector &v){
    //delete[] store;
    store = v.store;
    capacity = v.capacity;
    next_index = v.next_index;
}

You do the same thing.

when you call

const Vector Token::getLines() const{
    const Vector vec = lines;
    return lines;
}

vec = lines uses the copy constructor. You now have vec and lines pointing to the same store.

You return a copy of lines, this will trigger the copy constructor again. A third object now points to store.

When the stack unrolls, locally defined vec is destroyed. ~Vector deletes store. You now have two objects pointing to the same de-allocated store.

Kaboom! as soon as you try to do much of anything else with either of those Vectors. Looks like the destruction of the returned Vector hits first and causes the destructor to re-delete store.

You need to allocate storage for a new store and then copy the contents of source store into the new store in the = operator and the copy constructor.

Vector::Vector(const Vector &v){
    capacity = v.capacity;
    store=new int[capacity];
    for (size_t index; index < capacity; index++)
    {
        store[index] = v.store[index];
    }
    next_index = v.next_index;
}

and

Vector & Vector::operator =(const Vector &v){
    delete[] store;
    capacity = v.capacity;
    store=new int[capacity];
    for (size_t index; index < capacity; index++)
    {
        store[index] = v.store[index];
    }
    next_index = v.next_index;
}

std::copy can be used in place of the for loop in C++11. Hoary old memcpy can also be used, but only because store is a primitive data type.

And while I'm editing, thanks Jarod42, one more little tweak:

const Vector & Token::getLines() const{ //note the return of a reference. This avoids 
                                        // making a copy of lines unless the caller really 
                                        // wants a copy.
    // const Vector vec = lines; don't need to do this. lines is const-ified by the
    // const on the return type of the function
    return lines;
}

Answer 2

This error has nothing to do with calling a method with constant reference, but rather the function getLines(). For example, should you take hit1 and call the function getLines() directly, it will crash nonetheless. The issue is with how Token has a stack-allocated attribute Vector, which in turn has an int * attribute. This isn't necessarily an issue, but depending on how you implement those classes it can cause memory conflicts.

If you want to keep using your getLine() and can't use the <vector> libraries, you could change your Token's lines attribute to a Vector * and change all other syntax accordingly. Also remember to initialize your pointer lines memory or else it will crash.

However, I'd prefer to use as less dynamic-allocated memory as possible, if unnecessary. And like another user said, before while(temp->next != nullptr) you should have a condition if(temp != nullptr )