HiveBrain v1.2.0
Get Started
← Back to all entries
debugcppMinor

An attempt to make a linked list iterator "safe"

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
iteratormakeattemptsafelistlinked

Problem

I was practicing my C++ routine and decided to code up this simple linked list data structure. Furthermore, I made an attempt to make the list iterator "safe" in the sense that it throws an exception instead of tampering a memory region it should not..

safelist.h

```
#ifndef SAFE_LIST_H
#define SAFE_LIST_H

#include

namespace net {
namespace coderodde {
namespace util {

template
class Safe_list {
public:

Safe_list()
:
head{nullptr},
tail{nullptr}
{}

// Disable all the constructors + assignments:
Safe_list(const Safe_list&) = delete;
Safe_list(Safe_list&&) = delete;
Safe_list& operator=(const Safe_list&) = delete;
Safe_list& operator=(Safe_list&&) = delete;

void push_back(const T& datum)
{
Safe_list_node* new_node = new Safe_list_node(datum);

if (tail == nullptr)
{
head = new_node;
}
else
{
tail->next = new_node;
}

tail = new_node;
}

void pop_front()
{
check_list_not_empty();
Safe_list_node* removed = head;
head = head->next;

if (head == nullptr)
{
tail = nullptr;
}

delete removed;
}

const T& front() const
{
check_list_not_empty();
return head->datum;
}

T& front()
{
check_list_not_empty();
return head->datum;
}

private:

struct Safe_list_node {
T datum;
Safe_list_node* next;

Safe_list_node(const T& datum)
:
datum{datum},
next{nullptr}
{}
};

Safe_list_node* head;
Safe_list_node* tail;

void check_list_not_empty()
{
if (head == nullptr)
{
throw std::runtime_error{"Operating on an empty list."};
}
}

public:

class Safe_list_iterator {
Safe_list_node* current_node;

public:
Safe_list_iterator(Safe_lis

Solution

I mentioned that your code was leaking but it still is. There is no destructor to clean up the nodes.

To cleanly get proper cleanup you can use unique_ptr:

void pop_front()
{
    check_list_not_empty();
    Safe_list_node* next = head->next.release();
    head.reset(next);

    if (!head)
    {
        tail = nullptr;
    }
}

void push_back(const T& datum)
{
    std::unique_ptr new_node = std::make_unique(datum);
    tail = new_node.get();

    if (!tail)
    {
        head = std::move(new_node);
    }
    else
    {
        tail->next = std::move(new_node);
    }

}

struct Safe_list_node {
    T datum;
    std::unique_ptr next;

    Safe_list_node(const T& datum)
    :
    datum{datum},
    next{}
    {}
};

std::unique_ptr head;
Safe_list_node* tail;


Consider allowing (only) the move constructor and assignment. As is (with the unique_ptr) you can just remove the disabling declarations and it'll work, though I suggest implementing them yourself to null out the tail pointer in the moved-from object.

Code Snippets

void pop_front()
{
    check_list_not_empty();
    Safe_list_node* next = head->next.release();
    head.reset(next);

    if (!head)
    {
        tail = nullptr;
    }
}

void push_back(const T& datum)
{
    std::unique_ptr<Safe_list_node> new_node = std::make_unique<Safe_list_node>(datum);
    tail = new_node.get();

    if (!tail)
    {
        head = std::move(new_node);
    }
    else
    {
        tail->next = std::move(new_node);
    }

}

struct Safe_list_node {
    T datum;
    std::unique_ptr<Safe_list_node> next;

    Safe_list_node(const T& datum)
    :
    datum{datum},
    next{}
    {}
};

std::unique_ptr<Safe_list_node> head;
Safe_list_node* tail;

Context

StackExchange Code Review Q#146586, answer score: 5

Revisions (0)

No revisions yet.