0

I tried to create a very basic linked list using dynamic memory allocation.You can add elements and traverse the list.But I seem to be having problems while writing the destructor of the program(getting segmentation fault).

I instead took the code from my destructor and then put it in a member function called purge() which did not generate any errors.Why is this? Is there something fundamentally wrong with my class?

  #include <iostream>
struct LList
{
    LList *p , *s;
    int data;
    LList (LList *prev , LList *succ, int n) : p{prev} , s{succ} , data{n}{}
    ~LList()
    {

        auto start = this;
        while (start->p ) start = start->p;
        while(1)
        {
            auto temp = start;
            start = start->s;
            delete temp;
            if (start == nullptr) break;
        }
        std::cout << "Destruted!";
    }
    void traverse() const
    {
        auto start = this;
        while (start->p != nullptr)start = start->p;
        while (1)
        {
        std::cout << start->data;
        if (start->s == nullptr) break;
        else start = start->s;
        }
    }
};
int main()
{
    LList *natural = new LList{nullptr , nullptr , 1};
    natural = new LList{natural, nullptr , 2};
    natural->p->s = natural;
    natural = new LList{natural, nullptr , 5};
    natural->p->s = natural;
    natural->traverse();
    std::cout << natural->data;
    natural->~LList();
}

Version with Purge:

#include <iostream>
struct LList
{
   ....
    void purge()
    {

        auto start = this;
        while (start->p ) start = start->p;
        while(1)
        {
            auto temp = start;
            start = start->s;
            delete temp;
            if (start == nullptr) break;
        }
        std::cout << "Destruted!";
    }
    void traverse() const
    {
        auto start = this;
....
int main()
{
    ...
    natural->purge();
}

p.s : Does even purge do the trick ? I can still go:

natural->purge();
    std::cout << natural->data;
}

towards the end and it would output 125Disrupted!5

  • 2
    `natural->~LList();` -- Why are you doing this? Either the object dies naturally, or if dynamically allocated with `new`, a call to `delete` calls the destructor. – PaulMcKenzie Sep 30 '19 at 12:37
  • I was trying to explicitly call the destructor. –  Sep 30 '19 at 12:39
  • 2
    I know that is what you are trying to do. The question is "why?" Explicitly calling the destructor is seldom ever done, and its main purpose is used in conjunction with usage of `placement-new`, which you are not doing. Bottom line, that line of code is a mistake. – PaulMcKenzie Sep 30 '19 at 12:39
  • So how do I go about testing the destructor?It seems to give an error only when I call it.Why so? –  Sep 30 '19 at 12:46
  • `delete natural;` calls the destructor. In addition, there is no need to allocate everything. For example: `LList *natural = new LList{nullptr , nullptr , 1};` could have just been `LList natural{nullptr , nullptr , 1};`, and thus the destructor would have been called when the `natural` object goes out of scope. – PaulMcKenzie Sep 30 '19 at 12:47
  • @Aconcagua how? If `start->p` is a nullptr the loop won't run! –  Sep 30 '19 at 12:55
  • Also, it is your usage of dangling pointers in `main` that is preventing you from calling the destructor properly. You should be saving the first `natural` pointer to another variable so that you know where the head of the list is so that a `delete` call will go through the list starting from the head. – PaulMcKenzie Sep 30 '19 at 13:00
  • @PaulMcKenzie I tried `delete natural` but I get the same error.I am trying to emulate what Bjarne Stroutroup did in his book Programming Principles and Practice in Chapter 17 by using new and delete.I shall try what you suggested –  Sep 30 '19 at 13:01
  • See my previous comment. You should remember where the head of the list is and call `delete` on that. Your code in main essentially throws away the head pointer by reassigning it to `natural`, thus makes it difficult to retrieve later on at the end. `LList *natural = new LList{nullptr , nullptr , 1}; natural = new LList{natural, nullptr , 2};` -- ok, so where is the head node now, after those two lines are executed? See the issue? – PaulMcKenzie Sep 30 '19 at 13:02
  • @PaulMcKenzie I accounted for dangling pointer by doing `while (start->p ) start = start->p;` –  Sep 30 '19 at 13:05
  • You just implemented a node class for your linked list. Why don't you encapsulate that into another class maintaining the list as a whole? If done properly, that will safe you a lot of trouble. The tips @PaulMcKenzie gave would get implementation details of that outer list class. – Aconcagua Sep 30 '19 at 13:05
  • @ChiragM -- No, I am referring to attempting to destruct the list. You've "lost" the head node. Your main is calling delete on `natural`, but `natural` no longer points to the head. Plus the comment that you should implement a real linked list class is appropriate. – PaulMcKenzie Sep 30 '19 at 13:06
  • @PaulMcKenzie Could you implement it for me please? –  Sep 30 '19 at 13:07
  • @PaulMcKenzie Was struggling with that, too, at first. It's a doubly linked list, and what destructor does is iterating to tail node first and then from there backwards until head is passed... – Aconcagua Sep 30 '19 at 13:07
  • Cosmetics only: Wouldn't you agree that `do { ... } while(start);` looks more elegant than the if inside the `while(1)`? – Aconcagua Sep 30 '19 at 13:13
  • @PaulMcKenzie Just discovering: Calling the destructor explicitly, in given case, actually *is* correct! `auto start = this;`, then we iterate away from `this` towards begin of list, then from there forward to end, deleting all nodes on the way, *including `this`*! – Aconcagua Sep 30 '19 at 13:17
  • `natural->purge(); std::cout << natural->data;` is undefined behaviour: purge will delete all nodes, including the one `natural` points to. The object once living there is now gone, the pointer is dangling, and accessing the pointer now is undefined behaviour. You might be lucky (or unlucky, depending on point of view) and the memory the node once resided hasn't been given back to OS and thus still is physically accessible (and you'll read the last value written there), but you absolutely cannot rely on. – Aconcagua Sep 30 '19 at 13:31
  • Worse: Imagine in the meantime another new object was placed there. Then suddenly, this object gets modified unpredictably, just because you still write to `natural` – these are pretty difficult to find bugs... – Aconcagua Sep 30 '19 at 13:32
  • By the way (wondering why no one came up with earlier) – have you tried [debugging](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems)? – Aconcagua Sep 30 '19 at 13:35
  • 2
    The problem with this implementation is that the LList is attempting to represent both a node and simultaneously the entire list. The LList object should just mind its own business, and not try to claim ownership of its adjacent nodes. The destructor should not walk the list from the start and clean everything up. That is why it struggles with its node-ness identity and its list-ness identity. – Eljay Sep 30 '19 at 13:48

0 Answers0