0

I am working with legacy C code and the new code is written in C++. To use the C++ standard library, I wrote a simple Iterator for the legacy LinkedList as shown below after reading through Bjarne Stroustrup's blog post on Adaptation.

My question is:

  1. I want to create another Iterator for another struct say struct TokenList. I am not sure how to use namespace and still be able to use the range-based for loops. Any pointers would be helpful.

  2. Are the adapters for the Iterator namely: begin, end, ++, *, != correct? Currently, I'm an interested in reading the contents of the LinkedList using range-based for loops.

Coliru

#include <cstdio>
#include <numeric>
#include <algorithm>

struct LinkedList {
    double v;
    LinkedList *next;
};

struct Iterator {
    LinkedList *current;
    LinkedList &c;
};

Iterator begin(LinkedList *c) { return Iterator {c, *c}; }
Iterator end(LinkedList *c) { return Iterator {nullptr, *c}; }
Iterator &operator++(Iterator &p) { p.current = p.current->next; return p; }
LinkedList *operator*(Iterator p) { return p.current; }
bool operator!=(Iterator lhs, Iterator rhs) { return (lhs.current != rhs.current); }

int main()
{
    LinkedList *node1 = new LinkedList;
    LinkedList *node2 = new LinkedList;
    LinkedList *node3 = new LinkedList;

    node1->v = 1; node1->next = node2;
    node2->v = 2; node2->next = node3;
    node3->v = 3; node3->next = nullptr;

    printf("// C style: iteration\n");
    for (auto ptr = node1; ptr; ptr = ptr->next) {
        printf("%e\n", ptr->v);
    }

    auto head = node1;
    // make use of begin(), end(), ++, != and *
    printf("// Modern C++ style: range based for-loop\n");
    for (const auto& it : head) {
        printf("%e\n", it->v);
    }

    delete node3;
    delete node2;
    delete node1;

    return 0;
}
Anand
  • 1,122
  • 3
  • 20
  • 33
  • 1
    Implementing one's owned linked list is a useful programming excersize that's given in pretty much every introductory course on programming. But when one is faced with a real task, simply using `std::list` is much simpler. Can you clarify what specific problem you are trying to solve, using your own linked list implementation, that cannot be solved simply by using `std::list`, and everything that goes with it? – Sam Varshavchik Dec 09 '21 at 22:51
  • 1
    The old code seems to need some rework anyway. `LinkedList` is a _node_ in a linked list. Utterly confusing. – Ted Lyngmo Dec 09 '21 at 22:54
  • What does namespace have to do with this? Your iterator class should belong to your container, not the global scope level. The storage of a reference value in the iterator makes no sense. It's super sketchy. Don't do that. – paddy Dec 09 '21 at 22:57
  • 1
    Note also your deletion is incorrect. You cannot advance the iterator on a deleted node. – paddy Dec 09 '21 at 22:59
  • The best way to get a range-based for loop going would be to wrap the C-Style linked list in a a class that performed the same basic tasks as a library container with the same interface as used in the library containers. [Helpful link](https://stackoverflow.com/questions/7758580/writing-your-own-stl-container) – user4581301 Dec 09 '21 at 23:00

2 Answers2

4

Iterators are pseudo-pointer types. That means they themselves are regular.

struct Iterator {
  LinkedList *current;
  LinkedList &c;
};

Here you mix references and pointers. This is a serious anti-pattern, as what does assignment do? There is no sensible answer.

I would remove the c member entirely.

Next you need to broadcast an iterator type. Yours looks like a forward iterator. All end iterators can be equal.

Iterator begin(LinkedList *c) { return Iterator {c, *c}; }
Iterator end(LinkedList *c) { return Iterator {nullptr, *c}; }

These look ok. Just remove *c.

Note that the name does not have to be Iterator. begin/end must be defined in the namespace of LinkedList, but the return type does not have to be.

Iterator &operator++(Iterator &p) { p.current = p.current->next; return p; }

I usually implement this as a member function, and implement both pre and post increment; post is implemented using pre and copy.

LinkedList *operator*(Iterator p) { return p.current; }

This is wrong. It should return *p.current as a double&.

bool operator!=(Iterator lhs, Iterator rhs) { return (lhs.current != rhs.current); }

sure. Also implement == as !(lhs!=rhs).

Look up the forward iterator concept and forward iterator tag. Include the types needed for std::iterator_traits.

For other things to iterate, give the iterator a different name. This can be via a different namespace.

If the thing that differs is just the type of the value, you can make it a template pretty easy. Then you only have to manually write begin/end.

If the name of v also changes, you could use ADL on a GetValue(List*) function you write as a customization point.


Now, being usable in a ranged based for is different than being an iterator. Ranged based for is a tad easier; but the above upgrades you to a full forward iterator, which in turn reduces surprise when you try to use a std algorithm or basically anything else.


How I would write it:

// Iteration::LinkedListIterator<X> assumes that X is a linked list node
// with members ->next and ->value.  If it isn't, override the customization
// points GetNextNode and GetListElement in the namespace of X.

namespace Iteration {
  template<class List>
  List* GetNextNode( List* l ) {
    if (!l) return l;
    return l->next;
  }
  template<class List>
  decltype(auto) GetListElement( List* l ) {
    return l->value;
  }
  template<class List>
  struct LinkedListIterator {

    using self=LinkedListIterator;
    List *current;
    self& operator++(){ current = GetNextNode(current); return *this; }
    self operator++(int)&{ auto copy = *this; ++*this; return copy; }
    decltype(auto) operator*() {
      return GetListElement(current);
    }
    decltype(auto) operator*() const {
      return GetListElement(current);
    }
    auto operator->() {
      return std::addressof(GetListElement(current));
    }
    auto operator->() const {
      return std::addressof(GetListElement(current));
    }
    friend bool operator==(self const& lhs, self const& rhs) {
      return lhs.current == rhs.current;
    }
    friend bool operator!=(self const& lhs, self const& rhs) {
      return lhs.current != rhs.current;
    }

    using iterator_category = std::forward_iterator_tag;
    using value_type = std::decay_t<decltype(GetListElement(std::declval<List*>()))>;
    using difference_type = std::ptrdiff_t;
    using pointer = value_type*;
    using reference = value_type&;
  };
};

struct LinkedList {
    double v;
    LinkedList *next;
};

// customization point; the name of 
double& GetListElement( LinkedList* l ) { return l->v; }
double const& GetListElement( LinkedList const* l ) { return l->v; }
Iteration::LinkedListIterator<LinkedList> begin( LinkedList* l ) {
  return {l};
}
Iteration::LinkedListIterator<LinkedList> end( LinkedList* l ) {
  return {nullptr};
}
Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
1

Here is a small modification of Yakk's answer using C++20 concepts to find member functions for GetNextNode/GetListElement plus support for bidirectional iteration.

namespace Iteration {
    template<class List>
    List* GetNextNode(List* l) {
        if (!l) return l;
        return l->next;
    }
    template<class List>
    List* GetPreviousNode(List* l) {
        if (!l) return l;
        return l->prev;
    }
    template<class List>
    decltype(auto) GetListElement(List* l) {
        return l;
    }
    template<class T>
    concept HasGetNextNodeMember = requires(T a) {
        a.GetNextNode();
    };
    template<class T>
    concept HasGetPreviousNodeMember = requires(T a) {
        a.GetPreviousNode();
    };
    template<class T>
    concept HasGetListElement = requires(T a) {
        a.GetListElement();
    };
    template<class T>
    concept HasPrevious = requires(T a) {
        HasGetPreviousNodeMember<T> || a.prev;
    };

    template<class List>
    struct LinkedListIterator {
    private:
        static List* next(List* el)
        {
            if (!el) return el;
            if constexpr (HasGetNextNodeMember<List>)
                return el->GetNextNode();
            else
                return GetNextNode(el);
        }
        static List* prev(List* el)
        {
            if (!el) return el;
            if constexpr (HasGetPreviousNodeMember<List>)
                return el->GetPreviousNode();
            else
                return GetPreviousNode(el);
        }
        static auto element(List* el)
        {
            if constexpr (HasGetListElement<List>)
                return el->GetListElement();
            else
                return GetListElement(el);
        }
    public:
        using self = LinkedListIterator;
        List* current;

        self& operator++() { current = next(current); return *this; }
        self operator++(int)& { auto copy = *this; ++*this; return copy; }

        std::enable_if_t<HasPrevious<List>, self&> operator--() { current = prev(current); return *this; }
        std::enable_if_t<HasPrevious<List>, self&> operator--(int)& { auto copy = *this; ++*this; return copy; }

        decltype(auto) operator*() {
            return element(current);
        }
        decltype(auto) operator*() const {
            return element(current);
        }
        auto operator->() {
            return std::addressof(element(current));
        }
        auto operator->() const {
            return std::addressof(element(current));
        }
        friend bool operator==(self const& lhs, self const& rhs) {
            return lhs.current == rhs.current;
        }
        friend bool operator!=(self const& lhs, self const& rhs) {
            return lhs.current != rhs.current;
        }

        using iterator_category = std::conditional_t<HasPrevious<List>, std::bidirectional_iterator_tag, std::forward_iterator_tag>;
        using value_type = std::decay_t<decltype(element(std::declval<List*>()))>;
        using difference_type = std::ptrdiff_t;
        using pointer = value_type*;
        using reference = value_type&;
    };
};

You can use it like

struct LinkedList {
    double v;
    LinkedList *next;
};

double& GetListElement( LinkedList* l ) { return l->v; }
double const& GetListElement( LinkedList const* l ) { return l->v; }

or for bidirectional iterator

struct DoublyLinkedList {
    double v;
    DoublyLinkedList *next;
    DoublyLinkedList *prev;
};

double& GetListElement( DoublyLinkedList* l ) { return l->v; }
double const& GetListElement( DoublyLinkedList const* l ) { return l->v; }

or use a wrapper over the C class

struct DoublyLinkedList {
    double v;
    DoublyLinkedList *next;
    DoublyLinkedList *prev;
};

struct DoublyLinkedListWrapper : DoublyLinkedList {
    DoublyLinkedListWrapper* GetNextNode() const { return reinterpret_cast<DoublyLinkedListWrapper*>(next); }
    DoublyLinkedListWrapper* GetPrevNode() const { return reinterpret_cast<DoublyLinkedListWrapper*>(prev); }

    double& GetListElement() { return v; }
}