[Sorry, forgot to reply-all.]
---------- Forwarded message ----------
From: Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung(a)gmail.com>
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(a)hsr.ch>
On Fri, Nov 30, 2012 at 1:14 AM, Peter Sommerlad <peter.sommerlad(a)hsr.ch>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
