[boost] Fwd: [iterator] UB when implicitly using default constructed counting_iterator<unsigned>

Subject: [boost] Fwd: [iterator] UB when implicitly using default constructed counting_iterator<unsigned>
From: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2012-11-30 20:54:21

[Sorry, forgot to reply-all.]
---------- Forwarded message ----------
From: Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung_at_[hidden]>
Date: Fri, Nov 30, 2012 at 5:53 PM
Subject: Re: [boost][iterator] UB when implicitly using default constructed
counting_iterator<unsigned>
To: Peter Sommerlad <peter.sommerlad_at_[hidden]>

On Fri, Nov 30, 2012 at 1:14 AM, Peter Sommerlad <peter.sommerlad_at_[hidden]>wrote:

> Hi Jeffrey, hi all,
>
> I believe the usability of counting iterator could be improved, by
> value-initializing its Iterator type in the default constructor, because
> integral types won't be initialized by its default constructor. This can
> lead to hard to recognize user errors, e.g., when wrapping such a counting
> iterator with a filter iterator.
>
> counting_iterator is special in that sense, because to my understanding it
> is intended to adapt integral types to become iterators. And a default
> initialized scalar type, isn't.
>

"isn't"...what, exactly?

> I suggest a one line change:
>
>     counting_iterator() { }
>
> should become
>
>     counting_iterator() : super_t(Incrementable()) { }
>

Well, that's one thing you can do...

> The following example program shows the interesting UB, because the 3rd
> argument of make_filter_iterator, default constructs a
> counting_iterator<unsigned>().
>
> Any thoughts? Objections? Teachings...
>
> Regards
> Peter.
>
> =====>8
> #include <iostream>
> #include <boost/iterator/filter_iterator.hpp>
> #include <boost/iterator/counting_iterator.hpp>
> #include <algorithm>
> #include <iterator>
> #include <functional>
>
> struct is_prime{
>         bool operator()(unsigned int val) const {
>         if (val < 2) return false;
>         using std::placeholders::_1;
>         auto ce=boost::make_counting_iterator(val);
>         return ce==std::find_if_not(boost::make_counting_iterator(2u),ce,
>
> bind(std::modulus<unsigned>{},val,_1));
> }
> };
> int main(){
>
> std::copy_n(boost::make_filter_iterator(is_prime{},boost::make_counting_iterator(1u)
>         // if the following line is omitted we have undefined behavior,
> because of uninitialized unsigned value
>         //
>  ,boost::make_counting_iterator(std::numeric_limits<unsigned>::max()) //
> must be different from above value
>                         ),40,std::ostream_iterator<unsigned>{std::cout,",
> "});
> }
>
> =====8<

The thing that bothers me about this is that a default-constructed iterator
frequently is an end iterator; indeed, that's the assumption that
filter_iterator appears to be making if no end iterator is supplied. So if
the default-constructed iterator is not an end iterator, and you're
wrapping it with a filter_iterator and not explicitly supplying the end,
I'd say you're automatically asking for trouble, regardless of whether
you're using a counting_iterator or not. On the other hand, I'm not sure
what a sensible default value is for counting_iterator< [integral type] >;
0 is almost never going to be the end value; std::numeric_limits<>::max()
might be a better choice, but really the end value is context-dependent.

I'm not opposed to giving defined behavior to a default constructed
counting_iterator< [integral type] >, I'm just not sure what that defined
behavior should be. Maybe I'm over-analyzing it...

- Jeff