6

I have created a custom container called goldbox that only contains arithmetic types and I have also implemented begin and end member functions to iterate over the elements.

My Full Source Code:

#include <algorithm>
#include <vector>
#include <initializer_list>
#include <iostream>
#include <type_traits>
#include <ranges>

template <typename T>
concept Arithmetic = std::is_arithmetic_v<T>;

template <Arithmetic T = int>
class goldbox {
private:
    template <Arithmetic Base_t>
    struct Node {
        Base_t data;
        Node<Base_t>* prev;
        Node<Base_t>* next;
    };
    Node<T>* head;
    Node<T>* current_node;

    Node<T>*& __at_node(size_t index) {
        auto temp = head;
        size_t count {0};

        while (count < index) {
            temp = temp->next;
            count++;
        }
        current_node = temp;

        return current_node;
    }

    Node<T>*& __get_tail() {
        return __at_node(length() - 1);
    }

public:
    using value_type = T;

    goldbox() : head{nullptr}, current_node{nullptr} {}
    goldbox(std::initializer_list<T>&& list_arg) : goldbox() {
        decltype(auto) list_1 = std::forward<decltype(list_arg)>(list_arg);
        T temp[list_1.size()];
        std::copy(list_1.begin(), list_1.end(), temp);
        std::reverse(temp, temp + list_1.size());
        for (const auto& elem : temp)
            push_front(elem);
    }

    class iterator {
        private:
        Node<T>* node;
    public:
        iterator(Node<T>* arg) noexcept : node{arg} {}

        iterator& operator=(Node<T>* arg) {
            node = arg;
            return *this;
        }

        iterator operator++() {
            if (node)
                node = node->next;
            return *this;
        }

        iterator operator++(int) {
            iterator iter = *this;
            ++(*this);
            return iter;
        }

        iterator operator--() {
            if (node)
                node = node->prev;
            return *this;
        }

        iterator operator--(int) {
            iterator iter = *this;
            --(*this);
            return iter;
        }

        bool operator==(const iterator& other) {
            return (node == other.node);
        }

        bool operator!=(const iterator& other) {
            return (node != other.node);
        }

        T& operator*() {
            return node->data;
        }
    };

    iterator begin() {
        return iterator{head};
    }

    iterator end() {
        return iterator{nullptr};
    }

    size_t length() const {
        auto temp = head;
        size_t count {0};
        while (temp != nullptr) {
            ++count;
            temp = temp->next;
        }
        return count;
    }

    goldbox& push_front(T arg) {
        auto new_node = new Node<T>;

        new_node->data = arg;
        new_node->prev = nullptr;
        new_node->next = head;

        if (head != nullptr)
            head->prev = new_node;

        head = new_node;
        return *this;
    }

    goldbox& push_back(T arg) {
        auto new_node = new Node<T>;
        auto last = head;

        new_node->data = arg;
        new_node->next = nullptr;

        if (head == nullptr){
            new_node->prev = nullptr;
            head = new_node;
            return *this;
        }

        while (last->next != nullptr)
            last = last->next;
        last->next = new_node;
        new_node->prev = last;

        return *this;
    }

    goldbox& clear() {
        auto temp = head;
        Node<T>* next_temp;

        while (temp != nullptr) {
            next_temp = temp->next;
            delete temp;
            temp = next_temp;
        }

        head = nullptr;

        return *this;
    }

    goldbox& pop_back() {
        if (head != nullptr) {
            if (length() != 1) {
                delete std::move(__get_tail());
                __at_node(length() - 2)->next = nullptr;
            } else {
                this->clear();
            }
        }
        return *this;
    }

    goldbox& pop_front() {
        if (head != nullptr) {
            auto temp = head;
            head = head->next;
            delete temp;
        }
        return *this;
    }
};

int main() {
    goldbox goldbox_1 {2, 3, 5, 6, 7, 9};
    goldbox goldbox_2;

    for (const auto& elem : goldbox_1) {
        std::cout << elem << ' ';
    } std::cout << '\n';

    std::transform(goldbox_1.begin(), goldbox_1.end(), 
                   std::back_inserter(goldbox_2),
                   [](auto x){return 2 * x - 1; }
    );

    for (const auto& elem : goldbox_2) {
        std::cout << elem << ' ';
    } std::cout << '\n';

    return 0;
}

Output:

2 3 5 6 7 9
3 5 9 11 13 17

But I wanted to use it using ranges so that I don't have to create a new instance.

Once I applied the goldbox inside the range-based for loop:

for (const auto& elem: goldbox_1 | std::ranges::views::transform([](auto x){return 2 * x - 1;})) {
     std::cout << elem << ' ';
} std::cout << '\n';

It throws an error because I haven't provided an operator|.

If I used the non-pipe syntax:

for (const auto& elem: std::ranges::views::transform(goldbox_1, [](auto x){return x + 1;})) {
     std::cout << elem << ' ';
} std::cout << '\n';

It will still throw an error that both begin and end were not declared in the scope.

cigien
  • 57,834
  • 11
  • 73
  • 112
Desmond Gold
  • 1,517
  • 1
  • 7
  • 19
  • 1
    I'm a little curious about the reason you made your own container? While it can be a good exercise to do it a couple of times, most of the time the standard containers should suffice for most cases. – Some programmer dude May 01 '21 at 07:11
  • 1
    Yes, I replicated the standard container usage such as doubly linked list but that doesn't mean I will apply it for the projects but only as an exercise. – Desmond Gold May 01 '21 at 07:13
  • "*I have also implemented a subclass iterator with begin and end member functions.*" What is a "subclass iterator?" – Nicol Bolas May 01 '21 at 07:15
  • I'm sorry to confuse you but I mean the member class iterator inside my custom class or the `goldbox::iterator` constructor is used from `begin` and `end` member functions inside the `goldbox` class. – Desmond Gold May 01 '21 at 07:19
  • 4
    As for your problem, if the iterators are correct you shouldn't really have to do anything. Any iterable container (with `begin` and `end` member functions and properly implemented iterators) could be used as-is. Please try to create a [mcve] to show us, and include the full and complete build output in the question. – Some programmer dude May 01 '21 at 07:21
  • 2
    identifiers starting with double underscore are reserved – bolov May 01 '21 at 09:39
  • 2
    Please don't add updates that solve the problem you asked originally, when that invalidates existing answers. Go ahead and add an answer if you have a solution. If you have new questions that are not addressed by the existing answers, go ahead and post a new question. – cigien May 01 '21 at 14:13

1 Answers1

7

TLDR

Your class doesn't satisfy std::ranges::input_range because your iterator doesn't satisfy std::ranges::input_iterator. In your iterator class you need to:

  • add default constructor
  • add public difference_type alias
  • make pre increment and decrement operators return a reference
  • add difference operator
  • make operator== const
  • add public value_type alias
  • add T& operator*() const

it will still throw an error that both begin and end were not declared in the scope.

I do not know what compiler you use that gives this misleading error message. If you compile with gcc you get a proper error diagnostic:

note: the required expression 'std::ranges::__cust::begin(__t)' is invalid

  581 |         ranges::begin(__t);
      |         ~~~~~~~~~~~~~^~~~~

cc1plus: note: set '-fconcepts-diagnostics-depth=' to at least 2 for more detail

Setting a higher -fconcepts-diagnostics-depth= we can see the root causes:

note: no operand of the disjunction is satisfied

  114 |         requires is_array_v<remove_reference_t<_Tp>> || __member_begin<_Tp>
      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  115 |           || __adl_begin<_Tp>
      |           ^~~~~~~~~~~~~~~~~~~

Your container is not an array and you are not using adl begin but member begin so we need to look into why your class doesn't satisfy __member_begin:

note: 'std::__detail::__decay_copy(__t.begin())' does not satisfy return-type-requirement, because

  939 |           { __detail::__decay_copy(__t.begin()) } -> input_or_output_iterator;
      |             ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~

The problem is that your iterator class is not actually a proper iterator. Lets' see why:

note: the expression 'is_constructible_v<_Tp, _Args ...> [with _Tp = goldbox::iterator; _Args = {}]' evaluated to 'false'

  139 |       = destructible<_Tp> && is_constructible_v<_Tp, _Args...>;
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The first fix is to make your iterator default constructible. Doing that then recompiling we see that your class further doesn't satisfy the concept of an iterator:

note: the required type 'std::iter_difference_t<_Iter>' is invalid, because

  601 |         typename iter_difference_t<_Iter>;
      |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~

and

note: '++ __i' does not satisfy return-type-requirement, because

  603 |         { ++__i } -> same_as<_Iter&>;
      |           ^~~~~

You need to add difference_type public alias and make the pre increment and pre decrement operators return a reference to the iterator.

Fixing this then recompiling we see that your class further doesn't satisfy the concept of an iterator:

note: 'std::__detail::__decay_copy(__t.end())' does not satisfy return-type-requirement, because

  136 |           { __decay_copy(__t.end()) }
      |             ~~~~~~~~~~~~^~~~~~~~~~~

error: deduced expression type does not satisfy placeholder constraints

  136 |           { __decay_copy(__t.end()) }
      |           ~~^~~~~~~~~~~~~~~~~~~~~~~~~
  137 |             -> sentinel_for<decltype(_Begin{}(std::forward<_Tp>(__t)))>;
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

note: the required expression '(__t == __u)' is invalid, because

  282 |           { __t == __u } -> __boolean_testable;
      |             ~~~~^~~~~~

This means that the iterator returned by begin() is not comparable with the iterator returned by end. Looking down the diagnostic depth you can see that your operator== is not considered because it is not const.

Fixing this then recompiling we see that your class further doesn't satisfy the concept of an input_iterator:

note: the required type 'std::iter_value_t<_In>' is invalid, because

  514 |         typename iter_value_t<_In>;
      |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~

This is fixed by adding a public value_type alias.

Next:

note: nested requirement 'same_as<std::iter_reference_t, std::iter_reference_t<_Tp> >' is not satisfied, because

  517 |         requires same_as<iter_reference_t<const _In>,
      |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  518 |                          iter_reference_t<_In>>;
      |                          ~~~~~~~~~~~~~~~~~~~~~~

This means that operator* should return the same reference type for both iterator and const iterator. This is fixed by adding a const operator*:

T& operator*();
T& operator*() const;

And now all the compile errors are fixed and both versions (pipe and non-pipe) compile. Please note I've fixed the compile errors, haven't checked your semantics.

bolov
  • 72,283
  • 15
  • 145
  • 224
  • I'm having hard time rechecking my errors in my source code. When you put the summary of your answer, it eases my mind and now it finally compiles without any error. Thank you so much! <3. By the way, my compiler is also a GCC but I'm using the snapshot version which is 11.0.1. – Desmond Gold May 01 '21 at 11:41
  • 2
    You don't need to define `operator-`, just a member `difference_type`. – T.C. May 01 '21 at 13:13
  • 2
    For posterity, note that the statement "**The first fix is to make your iterator default constructible**" is correct today, but is considered a bug in the standard and this requirement will be lifted eventually. See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2325r3.html where the weakly_incrementable concept is discussed, and https://en.cppreference.com/w/cpp/iterator/weakly_incrementable – Nadav Har'El Mar 07 '22 at 15:06
  • I know this thread is pretty old, but I just wanted to say I appreciate you going through the steps for how to go about debugging such an issue. I'm dipping my toes in C++ waters for a project and was trying to implement a custom iterator for a class, and couldn't figure out why I couldn't use it with range adapters. Tl;Dr: Thank you – PatientPenguin Jul 31 '23 at 19:06
  • @PatientPenguin I really appreciate it! You are welcome and thank you! – bolov Jul 31 '23 at 20:42