0

I have a custom vector class that encapsulates the std::vector. I'm using it in a while loop to read through a CSV file line by line and store the columns into a Vector called columns and perform a bunch of operations on these values. Everything works fine, but after looping through some amount of lines it throws the error:

terminate called after throwing an instance of std::out_of_range

I'm assuming my Vector stops resizing for some reason, but it reads every line the way I want it to for a good amount of lines before it throws this error and stops. I use the std::cerr statements to see whether a couple of my column values are read correctly and using that, I can see that it stops before the end of the file. Why is this the case?

while(getline(datafile, line))
{
    string token;
    Vector<string> columns;
    WindLogType windlog2;
    stringstream ss(line);
    columns.add(string());
    while(getline(ss, token, ','))
    {
        columns.add(token);
    }
    stringstream date(columns[1]);
    string windspeed = columns[11];
    string solar1 = columns[12];
    cerr << solar1 << endl;
    string temperature1 = columns[18];
    cerr << temperature1 << endl; 
}

My Vector class:

#ifndef VECTOR2_H
#define VECTOR2_H
#include <iostream>
#include <string>
#include <sstream>
#include <vector>

using namespace std;

template <class T>
class Vector
{
public:
    Vector(){};
    ~Vector();

    void add(const T &obj);
    int vecsize() const{return data.size();}

    T& operator[](const int index);
    const T& operator[](const int index) const;

private:
    vector<T> data;

};

template <class T>
T& Vector<T>::operator[](int index){
    if(index < 0 || index > data.size()){
        throw("Out of bounds");
    }

    return data.at(index);
}

template <class T>
const T& Vector<T>::operator[](int index) const{
    if(index < 0 || index > data.size()){
        throw("Out of bounds");
    }

    return data.at(index);
}

template <class T>
Vector<T>::~Vector(){    
    data.clear();    
}

template <class T>
void Vector<T>::add(const T &obj){    
    data.push_back(obj);    
}

#endif // VECTOR_H
Azeem
  • 11,148
  • 4
  • 27
  • 40
thedafferg
  • 99
  • 7
  • 1
    Can you show us the relevant code from `Vector`? It's quite hard to help you with errors in your custom class, when you don't show this custom class ;) – Lukas-T May 17 '20 at 09:31
  • Learn to use the debugger or print the size of the vector which obviously is too small. – user2672165 May 17 '20 at 09:34
  • @churill Sorry about that. I've edited the post now that shows my vector class – thedafferg May 17 '20 at 09:55
  • Thanks for showing the code. Are you sure you are reading 18 columns? – Lukas-T May 17 '20 at 10:01
  • I believe I am. When I do an output statement for a column, the values show correctly for that column in the corresponding line the while loop is at. Its just that it stops after a certain line. For example, I can see that in my csv file for the last row the 'time' is 23:30 but the last bit of information that is output are the 'solar' and 'temperature' values at the line where time is 20:20. After that, the exception is thrown – thedafferg May 17 '20 at 10:06
  • 1
    It's probably best to use a debugger to inspect the values in the vector. Unrelated, but the condition of `operator[]` is off-by-one, should be ` if(index < 0 || index >= data.size())`, since `data.size()` is already out-of-bounds. – Lukas-T May 17 '20 at 10:19
  • 1
    @thedafferg: Run a debugging session and check if those indices exist (1, 11, 12, 18). The accessors are checking for bounds but omitting the case where `index == size()`. It should be `index >= 0 && index < size()`. But, you don't need to check the bounds either as the `std::vector::at()` method is already doing that for you and it throws an `std::out_of_range` exception that you're seeing. BTW, you don't need the ctor and dtor as the compiler provides those for you and `std::vector` has its own. And, preferably, use `std::size_t` for indices. Example: https://godbolt.org/z/g9d9xr. – Azeem May 17 '20 at 11:57

1 Answers1

1

It's generally advisable to avoid putting using namespace ...; in a header. This is probably especially so if you are implementing functionality that is already found in that namespace, such as a vector.

Instead of throw("str");, you should consider throwing an actual exception type, such as std::out_of_range or maybe even your own exception type. throw("str") is a little bit unusual, it means that the catch-site must use catch(const char *). It's not invalid, but, it's not common practice either.

In your T& Vector<T>::operator[](int) function (and const-qualified counterpart), you are checking whether the index is less than 0 (which is correct), but you are checking whether the index is greater than the internal vector's size. If you think of the case where the internal vector is empty, then that means that there are no valid indices at all (because there are no elements), but, your validation will allow index 0 through to the internal vector, because it is not greater than the size. If the internal vector has 1 element, then your validation allows both index 0 and index 1, which is not correct, it should allow only index 0. You can change this check to use index >= data.size() instead. This is called an "off-by-one" error.

Unless you plan on providing an implementation for negative indices, it's generally preferable to use size_t or another unsigned integer type.

Lastly, there is no need to explicitly call data.clear() in your destructor because the vector's destructor will already do this.

dreamlax
  • 93,976
  • 29
  • 161
  • 209
  • Thank you for your advice. Upon reading a comment above, its mentioned that I shouldnt even add a bound check in my operator overload as the std vector at function will do that for me anyway. So I'm wondering where would I add my catch and try, in the operator overload as well? – thedafferg May 18 '20 at 06:09
  • @thedafferg doing your own bounds checking is beneficial if you plan on changing the underlying vector implementation (e.g. from `std::vector` to another type that doesn't have bounds checking). If you will always use `std::vector` underneath, then the `.at()` function will automatically do bounds-checking for you, so there's no need to do it yourself. Whether you catch this in your own `operator[]` functions is largely a choice for you to make. Catching `std::out_of_range` yourself means that you can perform your own action in this case, otherwise, it is up to the caller to decide what to do – dreamlax May 18 '20 at 07:23