Re: [Boost-users] [Boost.Foreach] error iterating custom wrappers

From: Eric Niebler (eric_at_[hidden])
Date: 2008-05-13 11:28:03

Sorry for the delayed response. I was traveling last week, and didn't 
see this.

johny5.coder_at_[hidden] wrote:
> Hello to everybody.
> 
> Im using the great library Boost.Foreach and very happy with it,
> however currently I met some strange issue I want to show you.
> 
> Im using custom wrappers to iterate over mapped values in std::map<>
> container. Here is the wrapper:
> 
> template<typename CONT>
> struct Select2ndIt
> {
> 	CONT& cont;

OK, Select2nsIt is a container proxy.

> 	Select2ndIt(CONT& cont) : cont(cont) {}
> 
> 	typedef typename CONT::mapped_type  value_type;
> 
> 	struct iterator : public std::iterator
> 		<typename CONT::iterator::iterator_category, value_type, typename
> CONT::iterator::distance_type>
> 	{
> 		typename CONT::iterator  it;

Actually, this is not quite correct. See below.

> //		typedef typename CONT::iterator::difference_type  difference_type;
> 		typedef value_type*  pointer;
> 		typedef value_type&  reference;
> 
> 		iterator(typename CONT::iterator it) : it(it) {}
> 
> 		bool operator==(const iterator& other) const { return it == other.it; }
> 		bool operator!=(const iterator& other) const { return it != other.it; }
> 		iterator& operator++() { ++it; return *this; }
> 		iterator& operator++(int) { it++; return *this; }

Oops, that's wrong. Try this.

   iterator operator++(int) { iterator that(*this); ++it; return that; }

> 		iterator& operator--() { --it; return *this; }
> 		iterator& operator--(int) { it--; return *this; }

Ditto for operator--(int)

> 		reference  operator*() const { return it->second; }
> 		pointer   operator->() const { return &it->second; }
> 	};

So far so good. (Although you might simplify this code somewhat (and 
make it more correct) by using an iterator adaptor from the 
Boost.Iterators library.)

> 	struct const_iterator : public std::iterator
> 		<typename CONT::const_iterator::iterator_category, value_type,
> typename CONT::const_iterator::distance_type>
> 	{

<snip>

Here's the problem. Although it would seem to be correct to define 
separate iterator and const_iterator types in your container proxy, it's 
actually wrong. The const-ness of the iterators should match the 
const-ness of the underlying container, not the const-ness of the proxy. 
Consider the different meanings of the following:

   std::map<int,unsigned> m1;
   BOOST_FOREACH(unsigned& value, select2nd(m1)) // OK

   std::map<int,unsigned> const m2; // NOTE CONST HERE
   BOOST_FOREACH(unsigned const & value, select2nd(m2)) // OK

Whether the values in the map are const or not actually has nothing to 
do with whether the Select2ndIt proxy object is const. It's just a proxy.

So you can fix the issue simply by replacing the const_iterator 
definition with:

   typedef iterator const_iterator;

However, you'll also need to change the definition of your iterator 
struct. If CONT is const-qualified, iterator should adapt 
CONT::const_iterator instead of CONT::iterator.

HTH,

-- 
Eric Niebler
Boost Consulting
www.boost-consulting.com