Can't dereference const zip iterator about cppitertools HOT 11 CLOSED

Most of the tools don't have an operator*() const, reason being that it would place a requirement on the underlying iterable's iterator to have an operator*() const. Without the const at the itertool level, the underlying sequence can have either const or non-const operator*(). There's also not much you can do with a const iterator.

I should note that this code has other problems (which you may very well be aware of), you're calling begin on temporary. So the iterator you created is actually invalid before you've dereferenced it.o

$ grep "operator\*()" *.hpp
accumulate.hpp:    const AccumVal& operator*() const {
chain.hpp:    decltype(auto) operator*() {
chain.hpp:    iterator_deref<iterator_deref<ContainerT>> operator*() {
chunked.hpp:    DerefVec<ContainerT>& operator*() {
combinations.hpp:    CombIteratorDeref<ContainerT>& operator*() {
combinations_with_replacement.hpp:    CombIteratorDeref<ContainerT>& operator*() {
compress.hpp:    iterator_deref<ContainerT> operator*() {
cycle.hpp:    iterator_deref<ContainerT> operator*() {
dropwhile.hpp:    typename Holder::reference operator*() {
enumerate.hpp:    IterYield<ContainerT> operator*() {
filter.hpp:    typename Holder::reference operator*() {
groupby.hpp:    KeyGroupPair<ContainerT>& operator*() {
groupby.hpp:      iterator_deref<ContainerT> operator*() {
permutations.hpp:    Permutable<ContainerT>& operator*() {
powerset.hpp:    iterator_deref<CombinatorType<ContainerT>> operator*() {
product.hpp:    TupleDeref<TupleTypeT> operator*() {
range.hpp:    constexpr T operator*() const noexcept {
repeat.hpp:    constexpr const TPlain& operator*() const {
repeat.hpp:    constexpr const TPlain& operator*() const {
reversed.hpp:    reverse_iterator_deref<ContainerT> operator*() {
slice.hpp:    iterator_deref<ContainerT> operator*() {
sliding_window.hpp:    DerefVec<ContainerT>& operator*() {
starmap.hpp:    decltype(auto) operator*() {
starmap.hpp:    decltype(auto) operator*() {
takewhile.hpp:    typename Holder::reference operator*() {
zip.hpp:    TupleDeref<TupleTypeT> operator*() {
zip_longest.hpp:    ZipIterDeref<TupleTypeT, OptTempl> operator*() {

from cppitertools.

If you look at InputIterator you'll see that operator++ invalidates any other existing iterators, so your concern about saving a reference to *it is not alleviated by saving copies of the iterator. If this weren't already a problem I would suggest making the lambda mutable.

for (auto&& t : iter::zip(a, b, c)) {
  tasks_.emplace([this, t] { func(t); });
}

It seems like you're saving these tasks for later, but note that if you wanted to call them eagerly you could do so with iter::imap(f, a, b, c)

from cppitertools.

No problem. Good point there, in that case, a bit more complicated but you'd want to check the iterator_category and decide based on that. If it's not an InputIterator then copy

#include <cppitertools/zip.hpp>

#include <iostream>
#include <iterator>
#include <functional>
#include <tuple>
#include <type_traits>
#include <vector>

template <typename T>
void display_later(const T& seq) {
  using iter_type = typename std::iterator_traits<std::decay_t<decltype(seq.begin())>>::iterator_category;
  constexpr auto copy_element = std::is_same_v<iter_type, std::input_iterator_tag>;

  std::vector<std::function<void()>> funcs;
  for (auto&& t : seq) {
    if constexpr (copy_element || !std::is_lvalue_reference_v<decltype(t)>) {
      //std::cout << "capture value\n";
      funcs.emplace_back([t] { std::cout << std::get<0>(t) << '\n'; });
    } else {
      //std::cout << "capture reference\n";
      funcs.emplace_back([&t] { std::cout << std::get<0>(t) << '\n'; });
    }
  }

  for (auto&& f : funcs) {
    f();
  }
}



int main() {
  std::vector v{2, 4, 6};
  display_later(iter::zip(v));

  std::vector v_tups{std::tuple{2}, std::tuple{4}, std::tuple{6}};
  display_later(v_tups);
}

from cppitertools.

The reason I need to dereference a const iterator is that I want to capture the iterator in a lambda and call the lambda after advancing the iterator.

something like:
for (auto&& it = begin; it != end; ++it) { tasks_.emplace([this, it]() { func(*it); } }

The iterator inside the lambda is const.
I don't want to save a reference to *it because it might be invalid after calling ++it.
and I don't want to copy the return value of *it because it might be a reference to a large object.

Is copying the const iterator and then dereferencing it, a better solution?

from cppitertools.

Is there a way to know if I can keep a reference to *it or whether I need to make a copy?

Also, would should I do with this issue? close it? leave it open?

from cppitertools.

Don't worry about the issue. You can't keep a reference to *it because it's a temporary. However, if you're zipping up actual containers (like a few vectors, maps, etc) zip will be returning a tuple of references which isn't going to consume much space. I'd be really surprised if this was your bottleneck

from cppitertools.

Btw, the error message in both gcc and clang is very confusing.

Do you think something can be done about that?
I tried

static_assert(false, "not implement");

But I couldn't compile anything with that. maybe something a bit more clever could generate a compile time error only if the const operator* is actually called.

from cppitertools.

concepts will help with a lot of the issues in c++20 but I don't think this one can be made much better. a static_assert would have to use a dependent expression, something like static_assert((T*)nullptr, "not implemented") but there are so many things not implemented in itertools that I'd be adding hundreds of these everywhere and likely cause some compilers to start failing things they shouldn't be :/

from cppitertools.

if you're zipping up actual containers (like a few vectors, maps, etc) zip will be returning a tuple of references which isn't going to consume much space. I'd be really surprised if this was your bottleneck

You are correct that copying is not an issue when zip is used, because the zip iterator returns a tuple of references.
The problem is that the same same code might cause an expensive copy when standard containers are passed without using zip.

It looks like I need some kind of wrapper for standard collection and iterator that returns an std::reference_wrapper instead of a referece.

Something like:

template <class BaseIterator>
class ReferenceWrapperIterator : public BaseIterator {
 public:
  ReferenceWrapperIterator(BaseIterator&& other) : BaseIterator(std::move(other)) {}

  auto operator*() const { return std::ref(this->BaseIterator::operator*()); };
};

template <typename Collection>
class ReferenceWrapped {
 public:
  ReferenceWrapped(Collection& collection) : collection_(collection) {}

  auto begin() { return ReferenceWrapperIterator(collection_.begin()); };
  auto end() { return ReferenceWrapperIterator(collection_.end()); };

 private:
  Collection& collection_;
};

And then you can do

std::vector<int> v;
for (auto&& t : ReferenceWrapped(v) {
  tasks_.emplace([this] { func(t); });
}

Do you know if something like that already exists in cppitertools? or std?

from cppitertools.

Reference wrapper doesn't solve your problem because you're still binding a reference to whatever zip returns. You can't get a reference_wrapper to what zip's iterator is giving you, it's not any safer than a raw reference and std::ref won't even compile when I try to do it:

#include <cppitertools/zip.hpp>
#include <functional>

int main() {
  char v[] = {0};
  auto z = iter::zip(v);
  auto r = std::ref(*z.begin());
}

With a gcc saying:

zipref.cpp:7:31: error: use of deleted function ‘void std::ref(const _Tp&&) [with _Tp = std::tuple<char&>]’
   auto r = std::ref(*z.begin());

So, here is more like a straight forward demonstration of the problem:

#include <cppitertools/zip.hpp>

#include <iostream>
#include <functional>
#include <tuple>
#include <vector>

template <typename T>
void display_later(const T& seq) {
  std::vector<std::function<void()>> funcs;
  for (auto it = seq.begin(), end = seq.end(); it != end; ++it) {
    // get a reference to what the iterator points to
    auto&& t = *it;
    // hang onto that reference for later
    funcs.emplace_back([&t]  { std::cout << std::get<0>(t) << '\n'; });
  }

  for (auto&& f : funcs) {
    f(); // print get<0> of everything we have references to
  }
}



int main() {
  std::vector v{2, 4, 6};
  display_later(iter::zip(v)); // prints 6 6 6


  std::vector v_tups{std::tuple{2}, std::tuple{4}, std::tuple{6}};
  display_later(v_tups); // prints 2 4 6
}

The critical part here is that we have [&t] which is fine for the vector but for the zipped sequence we really want [t] to copy that tuple. Thankfully, we are living in an exciting time to be a c++ programmer. we can modify the loop to do the capture we want based on the type of t, add a #include <type_traits> at the top, and switch up the inner loop to get this:

#include <cppitertools/zip.hpp>

#include <iostream>
#include <functional>
#include <tuple>
#include <type_traits> // added this include here
#include <vector>

template <typename T>
void display_later(const T& seq) {
  std::vector<std::function<void()>> funcs;
  for (auto it = seq.begin(), end = seq.end(); it != end; ++it) {
    auto&& t = *it;
    // added the if constexpr here
    if constexpr (std::is_lvalue_reference_v<decltype(t)>) {
      //std::cout << "capture reference\n";
      funcs.emplace_back([&t] { std::cout << std::get<0>(t) << '\n'; });
    } else {
      //std::cout << "capture value\n";
      funcs.emplace_back([t] { std::cout << std::get<0>(t) << '\n'; });
    }
  }

  for (auto&& f : funcs) {
    f();
  }
}



int main() {
  std::vector v{2, 4, 6};
  display_later(iter::zip(v)); // now this prints 2 4 6 as well

  std::vector v_tups{std::tuple{2}, std::tuple{4}, std::tuple{6}};
  display_later(v_tups);
}

And you can also write it as a range-based for loop instead

  for (auto&& t : seq) {
    if constexpr (std::is_lvalue_reference_v<decltype(t)>) {
      //std::cout << "capture reference\n";
      funcs.emplace_back([&t] { std::cout << std::get<0>(t) << '\n'; });
    } else {
      //std::cout << "capture value\n";
      funcs.emplace_back([t] { std::cout << std::get<0>(t) << '\n'; });
    }
  }

from cppitertools.

we can modify the loop to do the capture we want based on the type of t
I suggested the same solution- capture by value for zipped and capture by reference for std iterators.
I guess i wasn't clear.

The only remaining issue I have is with iterators that return an lvalue reference to an internal storage in the iterator that is invalidated after advancing the iterator.

Such an iterators are valid, right?
Is there a way to detect such an iterator in compile time?

p.s. thanks for all your help, I really appreciate it.

from cppitertools.