Destructor Segmentation Fault Linked List

Question

So, I've searched forums up and down and either haven't found anything that helps me or haven't found anything that makes enough sense to connect it to the problem I'm having. My program stores Points and holds these points inside a struct (LNode) which is pointed to by my Cluster class creating a linked list. The only dynamic allocation is for the Lnodes and the value array of the Point objects. I am at a loss on what is causing my program to get a segmentation fault. I know from running the debugger that it happens when my function dynamically allocates a new LNode in the add function of my Cluster.cpp file. It happens when the function goes to exit and the point destructor is called. Here are the relevant parts of my files:

point.h

#include <iostream>

namespace Clustering {

class Point {
    unsigned int __id;
    int __dim;        // number of dimensions of the point
    double *__values; // values of the point's dimensions

    static unsigned int __idGen; // id generator

public:
    Point(int);
    Point(int, double *);

    // Big three: cpy ctor, overloaded operator=, dtor
    Point(const Point &);
    Point &operator=(const Point &);
    ~Point();

point.cpp

#include "Point.h"
#include <cmath>
#include <assert.h>

using namespace std;
using namespace Clustering;

namespace Clustering {

unsigned int Point::__idGen = 0;

Clustering::Point::Point(int i)
{
    __dim = i;
    __values = new double[__dim];
    for (int count = 0; count < __dim; ++count)
        __values[count] = 0.0;
    __id = __idGen++;
}

Point::Point(int i, double *pDouble)
{
    __dim = i;
    __values = pDouble;
    __id = __idGen++;
}

Point::Point(const Point &point)
{
    __dim = point.__dim;
    __values = new double[__dim];
    for (int count = 0; count < __dim; ++count)
        __values[count] = point.__values[count];
    __id = point.__id;
}

Point &Point::operator=(const Point &point)
{
    if (this == &point)
        return *this;
    else {
        __dim = point.__dim;
        for (int count = 0; count < __dim; ++count)
            __values[count] = point.__values[count];
        __id = point.__id;
    }
    return *this;
}

Point::~Point()
{
    std::cout << "This is the value " << &__values << std::endl;
    delete  [] __values;
}

Cluster.h

#include "Point.h"

namespace Clustering {

typedef struct LNode *LNodePtr;

struct LNode {

    Point point;
    LNodePtr next;
    LNode(const Point &p, LNodePtr n);

};

class Cluster {

    int __size;
    LNodePtr __points;

    //void __del();
    //void __cpy(LNodePtr pts);
    //bool __in(const Point &p) const;


public:
    Cluster();

    // The big three: cpy ctor, overloaded operator=, dtor
    Cluster(const Cluster &);
    Cluster &operator=(const Cluster &);
    ~Cluster();

    // Set functions: They allow calling c1.add(c2.remove(p));
    void add(const Point &);

    // Overloaded operators

    // Members: Subscript
    const Point &operator[](unsigned int index) const;

cluster.cpp

#include <cstdlib>
#include <assert.h>
#include "Cluster.h"

namespace Clustering{

LNode::LNode(const Point &p, LNodePtr n = nullptr) : point(0) {
    point = p;
    next = n;
}

Cluster::Cluster() {
    __size = 0;
    __points = nullptr;
}

Cluster::Cluster(const Cluster &cluster) {
    __size = cluster.__size;
    if(__size == 0)
        __points = nullptr;
    else{
        for(int count = 0; count < cluster.__size; ++count){
            add(cluster[count]);
        }
    }
}

Cluster &Cluster::operator=(const Cluster &cluster) {
    if(this == &cluster)
        return *this;
    else {
        for(int count = 0; count < cluster.__size; ++count){
            add(cluster[count]);
        }
    }
    return *this;
}

Cluster::~Cluster(){
    if(__points != nullptr){
        LNodePtr currPtr = __points;
        LNodePtr nextPtr = nullptr;
        while(currPtr != nullptr){
            nextPtr = currPtr->next;
            delete currPtr;
            currPtr = nextPtr;
        }
    }
    else
        assert(__size==0);
}
void Cluster::add(const Point &point) {
    Point p(point);
    LNodePtr insertPtr = new LNode(p, nullptr);
    LNodePtr prev = __points;
    LNodePtr next = __points;
    if(__points == nullptr) {
        __points = insertPtr;
        __size++;
    }
    else if(__points->next == nullptr){
         if (point < __points->point) {
            __points = insertPtr;
            __size++;
        }
        else
            __points->next = insertPtr;
    }
    else{
        while(next != nullptr && (prev->point < point && point >= next->point)){
            prev = next;
            next = next->next;
        }
        prev->next = insertPtr;
    }
}
 const Point &Cluster::operator[](unsigned int index) const {
    assert(__points != nullptr && index < __size);
    LNodePtr cursor = __points;
    for(int count = 0; count < index; ++count)
        cursor = cursor->next;
    return cursor->point;
}

and in main, a simple test like this is what I use.

Cluster c1;
c1.add(Point(5));

Any help is greatly appreciated, I am fairly new to this type of structure and have been dealing with this error for a solid part of 2 days now.

Answer 1

In the Cluster::add function you create a new LNode and in the LNode constructor the point object is constructed with a zero sized array. Which is allowed but gives undefined behavior. See C++ new int[0] -- will it allocate memory?. You have created a zero sized array, but in the Point::operator= function, which is used in the first line of the LNode constructor, you never allocate new memory for __values so you are copying 5 values into a zero sized array. Who knows what you're overwriting when that happens. Try running with valgrind or some other memory analysis tool. That should identify this problem and any others.

Answer 2

Michael covered the segmentation violation problem; there is a smaller, logical problem in the add() method: the method never updates insertptr->next, so wherever the new node is inserted, all following already existing nodes are lost.