C++: std::equal segmentation fault with my container

Question

I'm implementing a container like std::vector, with the aim of learning C++ and its idioms.

With the purpose of overloading the operator ==, i first wrote my own version:

bool operator==(const Vector& other) const{
    if (_size == other._size) {
        for (int i = 0; i < _size; i++)
            if (!(_elements[i] == other[i]))
                return false;
        return true;
    }
    return false;
}

Then i defined the operator as a free function and simplified the implementation as follows:

template <typename T>
bool operator==(const Vector<T>& first, const Vector<T>& second){
    std::cout << "Comparing" << std::endl;
    return std::equal(first.begin(), first.end(), second.begin(), second.end());
}

My first implementation works great, but the second gives a segmentation fault.

The implementations are tested with this example:

#include <iostream>
#include "vector.hpp"
int main(int argc, char** argv) {
    Vector<int> v;
    Vector<int> v1;

    for (int i = 0; i<5; i++)
        v1.push_back(i);

    v = v1;
    for (auto i : v)
        std::cout << i << std::endl;

    if (v == v1)    // This line gives segfault
        std::cout << "Equal" << std::endl;
    else
        std::cout << "Different" << std::endl;

    return 0;
}

The last string that is being printed is "Comparing" (printed by the operator ==), then i get the segmentation fault.

This is an overview of my Vector implementation:

template <typename T>
class Vector {
private:
    T* _elements = nullptr;
    size_type _capacity = 0;
    size_type _size = 0;
public:
    Vector () = default;

    // Copy constructor
    Vector (const Vector& other) {

        _capacity = other.capacity();
        if(_elements)
            delete[] _elements;
        _size = other.size();
        _elements = static_cast<T*>((void*)new char[_capacity*sizeof(T)]);
        for(int i = 0; i < _size; i++)
            new(&_elements[i]) T(other[i]);

    }

    // Copy Assignment
    Vector& operator=(Vector other){

        swap(*this, other);
        return *this;

    }

    template<typename U>
    friend void swap(Vector<U>& first, Vector<U>& second);

    template<typename U>
    friend bool operator==(const Vector<U>& first, const Vector<U>& second);

    T* begin(){
        return _elements;
    }

    T* end(){
        return &_elements[_size];
    }

    T* begin() const{
        return begin();
    }

    T* end() const{
        return end();
    }

    void push_back(const T& e){

        if(_size >= _capacity)
            reserve(1+_capacity*2);

        new (&_elements[_size++]) T{e};

    }
    ...
};

template<typename T>
void swap(Vector<T>& first, Vector<T>& second){

    using std::swap;
    // Unqualified function call resolved by the ADL
    swap(first._capacity, second._capacity);
    swap(first._size, second._size);
    swap(first._elements, second._elements);

}

template <typename T>
bool operator==(const Vector<T>& first, const Vector<T>& second){
    std::cout << "Comparing" << std::endl;
    return std::equal(first.begin(), first.end(), second.begin(), second.end());
}

I can't figure out what i'm doing wrong. Let me know if more details are needed.

Thanks in advice!

P.S.: I'm compiling with clang++ -std=c++14

Answer 1

Your problem is an infinite loop in the const versions of begin and end:

T* begin() const{
    return begin();
}

That loop will produce the segmentation fault when the stack size is exhausted. It is only called in the second version of your operator==, the first version does not trigger it.

Just replace the implementation with the same implementation you used for the non-const versions:

T* begin() const{
    return _elements;
}

T* end() const{
    return &_elements[_size];
}

You might also think about const_iterator, e.g., returning const T* instead of T* for some of your methods - but that is another topic for another time :)