Creating a custom vector class. Push_back function only working for the first value

0
votes

In my Comp Sci class, we are learning how to make our own vector class. We will eventually store our custom made string class objects in a custom made vector class. I wanted to try and build a vector class of integers beforehand for simplicity.

So far, I have a default constructor that initializes my pointer to an empty array and sets the size to 0. Then I try to append some values using my push_back function and then check to make sure it was done correctly.

When I do std::cout << v[0] << std::endl;

I get the correct output (10). However, if I call push_back again and then call v[1] I get 0.

I feel like I am not allocating memory correctly in my push_back function but I am not sure.

Thanks for any advice!

[part 1][1]

[part 2][2]

sorry if my formatting is wrong I am new to posting here.

class:

class myVector
{
private:
    int *data; //will point to an array of ints
    size_t size; //determins the size of array
public:
    myVector(); // default constructor
    void push_back(int); // appends an integer to the vector
    int operator[](size_t);
    size_t sizeOf();
};

main:

int main()
{
    myVector v;
    v.push_back(10);
    std::cout << v.sizeOf() << std::endl;
    v.push_back(14);
    std::cout << v.sizeOf() << std::endl;
    std::cout << v[1] << std::endl;

    return 0;

}

member functions:

size_t myVector::sizeOf()
{
    return size;
}

int myVector::operator[](size_t location)
{
    return this->data[location]; //this will return the value at data + 
                                 //location
}

myVector::myVector()
{
    this->data = new int[0]; //initialize the data to an empty array of 
                             //ints
    size = 0; //initialize the size to 0
}

void myVector::push_back(int val)
{
    if(size == 0) //if size == 0, create a new array with 1 extra index
    {
        ++size;
        delete [] this->data;
        this->data = new int[size];
        this->data[0] = val;
    }
    else
    {
         ++size;
         int *temp = new int[size - 1];
         for(int i = 0; i != (size - 1); i++)
         {
              temp[i] = this->data[i];
         }
         delete [] this->data;
         this->data = new int[size];
         for(int i = 0; i != (size - 1); i++)
         {
             this->data[i] = temp[i];
         }
         this->data[size] = val;
         delete [] temp;
    }
}
3
c++arraysclasspointersdynamic-memory-allocation
please include a minimal reproducible example in the question. Code as text not as images please - 463035818_is_not_a_number
I apologize, trying to get the formatting correct enough to submit was annoying. But now its up! - user12633830
you start with size=0 then you increment size and allocate an array via new int[size -1], thats still not big enough for 1 element. - 463035818_is_not_a_number
@idclev463035818 That's actually not the problem, because there's a second allocation this->data = new int[size] later on. There's way too many uses of new and delete[] in this code, but it's not actually causing this specific issue. - Nathan Pierson
@NathanPierson oh right thanks. What is saw is just to store the elements in some temp array (for whatever reason) I just saw that and didnt read on. - 463035818_is_not_a_number

3 Answers

2
votes

In your code:

this->data[size] = val;

you are going outside of the allocated array.

Same in the previous loop (in its last iteration):

 for(int i = 0; i != (size - 1); i++)
 {
     this->data[i] = temp[i];
 }
0
votes

There are a few problems.

  1. It does not look like you need a special case for 0 sized vector

  2. you do not allocate enough memory:

Example, if size is one, you hit this case, then size becomes 2, and you allocate a buffer of ... 1.

else
{
     ++size;
     int *temp = new int[size - 1];
     for(int i = 0; i != (size - 1); i++)
     {
         temp[i] = this->data[i];
     }

Tip: use ```for (int i = 0; i < size; ++i)```  and ```new int[size]```

  1. you go out of bounds after your loop. If you allocate [size] bytes, then (size-1) is the last valid index.

  2. you copy data into temp, then copy temp into ANOTHER allocation. You don't need to do that. Just assign this->data = temp; The whole second loop is needless, and don't delete temp at the end.

0
votes

Its not necessary to a lot of new and delete operations and loops. I fixed and cleaned your two functions.

myVector::myVector()
{
    this->data = new int[1]; //initialize the data to an empty array of 
                             //ints
    size = 0; //initialize the size to 0
}

void myVector::push_back(int val)
{
    if(size == 0) //if size == 0, create a new array with 1 extra index
    {
        ++size;
        this->data[0] = val;
    }
    else
    {
         ++size;
         int *temp = new int[size];
         for(int i = 0; i != (size-1); ++i)
         {
              temp[i] = this->data[i];
         }
         delete [] this->data;
         this->data = temp;
         this->data[size-1]=val;
    }
}

in push_back function allocate a new array with new size and copy data from existing array. After deleting existing array and we see this->data can't point to valid location. Assign the new array's address to this->data and we access existing data and size increased +1. Last we assign parameter val to end of array(size-1).