Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

counting_iterator is not default constructible (ForwardIterator requirement) #22

Closed
fenbf opened this issue Nov 22, 2018 · 6 comments
Closed

Comments

@fenbf
Copy link

fenbf commented Nov 22, 2018

counting_iterator is very useful when you want to access several containers during parallel execution.

The iterator (based on TBB) is modelled as random access iterator, but it's missing a default constructor (that is required by ForwardIterator type - it's "base" class as an iterator)
https://en.cppreference.com/w/cpp/named_req/ForwardIterator

is it easy to fix the issue and provide such default constructor?

it looks like ZipIterator has the same issue

I've noticed that issue when I tried to use counting_iterator with MSVC parallel stl implementation, as MSVC needs to default construct an iterator.

@akukanov
Copy link
Contributor

Adding default constructors should be easy; we will do it in a future TBB update.

@BillyONeal
Copy link

BillyONeal commented Nov 27, 2018

Default constructors are insufficient to make counting_iterator acceptable for use in standard parallel algorithms. At the moment, counting_iterator isn't a forward iterator, much less a random access iterator. This is because counting_iterator is stashing -- that is, it returns a reference to the contained integer value, it doesn't return a reference to an integer stored in some other container. That doesn't meet the "equal iterators reference the same object" forward iterator requirement ( http://eel.is/c++draft/forward.iterators#6 ).

The practical differences of this are that:

  1. Parallel algorithms libraries are allowed to assume iterator differences fit into a size_t, since any object they reference must be in memory, and size_t can index into any memory. MSVC++'s parallel algorithms implementation does this because all the underlying platform APIs that talk about threads want size_ts, not iterator difference types, and the potential for integer overflow bugs when those get mixed up is high.
  2. Iterator adapters that destroy the original iterator, like std::reverse_iterator, cause undefined behavior when used with counting_iterator, so if the parallel algorithm uses std::reverse_iterator anywhere things are doomed.

It's possible to make a parallel algorithms implementation that works with stashing iterators like this, but the standard doesn't currently require that to work (and it doesn't work on our implementation (MSVC++) at present).

Billy3

@akukanov
Copy link
Contributor

I hope that the need for fancy iterators will be greatly reduced, if not fully eliminated, with the range based approach and all the variety of views that e.g. Range-V3 library provides.

The TBB fancy iterators do not pretend to be fully standard-compliant (and I think this is true for Boost analogues as well); these iterators work where they work, and still are quite useful. We have no goal to make those work with every C++ algorithm, but we will implement reasonable improvements to fix issues for particular use cases, like this one.

@BillyONeal
Copy link

The use case was to use counting_iterator with the standard parallel algorithms; I'm saying that won't always work even with the default ctor changes.

@akukanov
Copy link
Contributor

Absolutely, and I think we all agree on this.

@tbbdev
Copy link
Contributor

tbbdev commented Apr 21, 2020

Default constructor for counting iterator was added in TBB 2019 U4.

@tbbdev tbbdev closed this as completed Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants