1

Hi Currently i have been honing my skill in the data structure so that i can become a good Game Developer, i am Learning Linked list and made a Linked list Program with Insertion ,Deletion and Recursive insertion and Reversing a Linked list Can you Guys tell me am i here Clearing the assigned memory Created with the new Operator Correctly, I am getting the Desired Output but i am worried about memory leak. ... please be gentle aim Still learning.

class Node
{
    int data;
    Node *Next;

public:
    int GetData()
    {
        return data;
    }
    void SetData(int Data)
    {
        data = Data;
    }

    Node *GetNext()
    {
        return Next;
    }

    void SetNext(Node *next)
    {
        Next = next;
    }
};

void Insert(Node **Head, int x)
{
    Node *temp = new Node();

    temp->SetData(x);
    temp->SetNext(*Head);

    *Head = temp;
}

void InsertAt(Node **Head, int x, int n)
{
    if (n == 0)
    {
        std::cout << "The Given data at 'n' cannot be assigned to null\n";
    }

    Node *temp = new Node();
    temp->SetData(x);
    if (n == 1)
    {
        temp->SetNext(nullptr);
        *Head = temp;
        return;
    }

    Node *temp2 = *Head;
    if (Head == nullptr)
    {
        std::cout << "The Given data cannot be assigned to null\n";
    }

    for (int i = 0; i < n - 2; i++)
    {
        temp2 = temp2->GetNext();
    }

    temp->SetNext(temp2->GetNext());
    temp2->SetNext(temp);
}

void AppendList(Node **Head, int Data)
{
    Node *temp = new Node();

    Node *LastNode = *Head;

    temp->SetData(Data);
    temp->SetNext(nullptr);

    if (*Head == nullptr)
    {
        *Head = temp;
        return;
    }
    // else Traverse till last node.

    while (LastNode->GetNext() != nullptr)
    {
        LastNode = LastNode->GetNext();
    }
    LastNode->SetNext(temp);
}

void DeleteNode(Node **Head, int n)
{
    Node *temp = *Head;

    if (n == 1)
    {
        *Head = temp->GetNext();
        std::cout << "\nAfter Deletion of Head Node\n";
        return;
    }

    for (int i = 0; i < n - 2; i++)
    {
        temp = temp->GetNext();
    }
    Node *temp2 = temp->GetNext();
    temp->SetNext(temp2->GetNext());
    std::cout << "After Deletion of Node\n";
}

Node *ReverseList(Node *Head)
{
    Node *Current, *Prev, *next;

    Current = Head;
    Prev = nullptr;
    while (Current != nullptr)
    {
        next = Current->GetNext();
        Current->SetNext(Prev);
        Prev = Current;
        Current = next;
    }
    Head = Prev;
    return Head;
}

int LinkedList_Count(Node *Head)
{
    int count = 0;

    Node *Current = Head;
    while (Current != nullptr)
    {
        count++;
        Current = Current->GetNext();
    }
    std::cout << "Number of Elements: " << count;
    return count;
}

void PrintList(Node *Head)
{
    std::cout << "Data list : ";

    while (Head != nullptr)
    {
        std::cout << " " << Head->GetData();
        Head = Head->GetNext();
    }
    std::cout << "\n";
}

//Print Listed using Recursion
void Recursion_Print(Node *Head)
{
    if (Head == nullptr)
    {
        return;
    }

    std::cout << ' ' << Head->GetData(); //comment to Do Reverse the Linked list
    Recursion_Print(Head->GetNext());
    //std::cout << ' ' << Head->GetData();//unComment to Reverse the linked List recursively
}

Node *RecursiveRevList(Node *Head)
{
    Node *temp;
    if (Head->GetNext() == nullptr)
    {
        temp = Head;
        return temp;
    }
    temp = RecursiveRevList(Head->GetNext());
    Head->GetNext()->SetNext(Head);
    Head->SetNext(nullptr);
    return temp;
}
void RunList()
{
    Node *Head = NULL;

    //AppendList(&Head, 16);
    Insert(&Head, 6);
    Insert(&Head, 7);
    Insert(&Head, 8);
    InsertAt(&Head, 18, 2);
    std::cout << "Data list : \n";
    Recursion_Print(Head);
    std::cout << '\n';
    LinkedList_Count(Head);
    DeleteNode(&Head, 1);
    //Head = ReverseList(Head);
    Head = RecursiveRevList(Head);
    PrintList(Head);
    LinkedList_Count(Head);
    delete Head;
}
  • 3
    You certainly have a leak, since you never free memory, nor do you use a smart pointer that will free memory automatically. In particular your `DeleteNode` detaches a node from the list but never deletes it. – Ben Voigt Mar 31 '21 at 16:39
  • 4
    The best way to check if there are no memory leaks is to write extensive tests and run them under leak-detecting tool like valgrind. It's hard to detect memory leaks just by looking at code, especially if it's not conforming to RAII principle. – Yksisarvinen Mar 31 '21 at 16:39
  • 4
    See also [this C++ reference](https://en.cppreference.com/w/cpp), read about [standard containers](https://en.cppreference.com/w/cpp/container) and the [C++ rule of five](https://cpppatterns.com/patterns/rule-of-five.html). Compile with [GCC](http://gcc.gnu.org/) invoked with at least `g++ -Wall -Wextra -g`. Take inspiration from existing open source C++ projects (e.g. [fish](http://fishshell.com/), [RefPerSys](http://refpersys.org/), [ninja](http://ninja-build.org/)...) – Basile Starynkevitch Mar 31 '21 at 16:41
  • 1
    Where does DeleteNode free the node that is removed? – drescherjm Mar 31 '21 at 16:41
  • 4
    Unrelated and more of a personal reflection, but the resource or teacher you use to learn C++ might be doing a bad job of teaching C++. You clearly know about classes, so why don't you use classes for the list itself? Making all the global functions member functions of e.g. a `List` class? – Some programmer dude Mar 31 '21 at 16:42
  • 1
    Linked list is one of the few places I would use `new`, particularly if I knew the list will be long enough to threaten stack overflow. In this case there's already recursive iteration, so might as well use the `unique_ptr`. In general Here's an excellent presentation on C++'s smart pointers: [Leak-Freedom in C++... By Default.](https://www.youtube.com/watch?v=JfmTagWcqoE) If I'm not mixing up presentation, sI beleve this is the one where Mr. Sutter uses linked list as an example. – user4581301 Mar 31 '21 at 17:08
  • [Related: The **Community addition** in this linked answer](https://stackoverflow.com/a/22122095/4581301) demonstrates a way to use an extra layer of indirection to dramatically simplify the book-keeping required when modifying, in this case removing a node, a linked list. – user4581301 Mar 31 '21 at 17:12

2 Answers2

4

You're writing C-style code; in C++, you should avoid explicit calls to new. Your example is far too long to rewrite, but here is a very small start:

#include <memory>
class Node
{
    int data;
    std::shared_ptr<Node> Next;

// ...
void Insert(std::shared_ptr<Node>& Head, int x)
{
    auto temp = std::make_shared<Node>();
    // ...
}

(Note that std::unique_ptr is probably a better choice than std::shared_ptr, but that incurs complications with copying Node of which you're "blissfully" unaware right now.)

And, in practice, you should really use std::list (in your case, std::list<int>) rather than writing your own. Once you're proficient using std::list (and friends like std::vector), you'll be better able to "roll your own."

Ðаn
  • 10,400
  • 11
  • 57
  • 90
  • 4
    @TricksterRazor Ðаn's point is valid, though. It boils down to *manual memory management is NOT basic.* It is an intermediate-to-advanced level skill, and one of the harder things to get right in programming. Even for the experts, so do what the experts do: Use it extremely rarely. – user4581301 Mar 31 '21 at 17:38
2

As pointed out in the comments by many learned people, you have a memory leak in your program. When you are deleting the nodes, you're not actually freeing the allocated memory locations. The correct way? Use delete to deallocate the memory from the program.

I would recommend you to learn it as a rule of thumb, when programming in C or C++, if you're allocating dynamic memory somewhere in your program, then in all certainty you'd have some deletion method where you should use free() or delete to deallocate the memory from the heap.

Here are some resources which might help you.

  • 2
    Handy reading: [Why should C++ programmers minimize use of 'new'?](https://stackoverflow.com/questions/6500313/why-should-c-programmers-minimize-use-of-new) – user4581301 Mar 31 '21 at 17:02