1
\$\begingroup\$

I've reworked (for C++11) a section of PyCXX which contained an almost identical iterator and const_iterator.

I would like to know whether there is any way I can further tidy the code.

(Please bear in mind that the original was written maybe 20 years ago).

Here is the original:

template<TEMPLATE_TYPENAME T>
class SeqBase: public Object
{
public:
    // :
    // UNTOUCHED
    // :

    /* from Config.hxx:
            #if defined( _MSC_VER )
            #   define STANDARD_LIBRARY_HAS_ITERATOR_TRAITS 1
            #elif defined( __GNUC__ )
            #   if __GNUC__ >= 3
            #       define STANDARD_LIBRARY_HAS_ITERATOR_TRAITS 1
            #   else
            #       define STANDARD_LIBRARY_HAS_ITERATOR_TRAITS 0
            #   endif
            //  Assume all other compilers do
            #else
            #   define STANDARD_LIBRARY_HAS_ITERATOR_TRAITS 1
            #endif

            #if STANDARD_LIBRARY_HAS_ITERATOR_TRAITS
            #  define random_access_iterator_parent(itemtype) std::iterator<std::random_access_iterator_tag,itemtype,int>
            #else
            #  define random_access_iterator_parent(itemtype) std::random_access_iterator<itemtype, int>
            #endif
     */

    class iterator
    : public random_access_iterator_parent( seqref<T> )
    {
    protected:
        friend class SeqBase<T>; // allow SeqBase<T> to see our data
        SeqBase<T> *seq;
        sequence_index_type count;

    public:
        iterator()                           : seq{ 0 }         , count{ 0 }            { }
        iterator( SeqBase<T> *s, int where ) : seq{ s }         , count{ where }        { }
        iterator( const iterator& other )    : seq{ other.seq } , count{ other.count }  { }

        ~iterator()                                                                     { }

        seqref<T> operator*()                           { return seqref<T>( *seq, count );  } // the value this iterator is pointing to
        seqref<T> operator[]( sequence_index_type i )   { return seqref<T>( *seq, count + i );  }

        iterator& operator=( const iterator& other ) {
            if( this != &other ) {
                seq = other.seq;
                count = other.count;
            }
            return *this;
        }

        bool eql( const iterator& other ) const  { return seq->ptr() == other.seq->ptr() && count == other.count;  }
        bool neq( const iterator& other ) const  { return seq->ptr() != other.seq->ptr() || count != other.count;  }
        bool lss( const iterator& other ) const  { return                                   count <  other.count;  }
        bool gtr( const iterator& other ) const  { return                                   count >  other.count;  }
        bool leq( const iterator& other ) const  { return                                   count <= other.count;  }
        bool geq( const iterator& other ) const  { return                                   count >= other.count;  }

        iterator  operator +  ( int n )     const  { return iterator( seq, count + n ); }
        iterator  operator -  ( int n )     const  { return iterator( seq, count - n ); }
        iterator& operator += ( int n )            { count += n; return *this; }
        iterator& operator -= ( int n )            { count -= n; return *this; }

        int operator-( const iterator& other ) const {
            if( seq->ptr() != other.seq->ptr() )
                throw RuntimeError( "SeqBase<T>::iterator comparison error" );
            return count - other.count;
        }

        // prefix / postfix
        iterator& operator++() { count++; return *this; }   iterator operator++( int ) { return iterator( seq, count++ ); }
        iterator& operator--() { count--; return *this; }   iterator operator--( int ) { return iterator( seq, count-- ); }

        std::string diagnose() const
        {
            std::OSTRSTREAM oss;
            oss << "iterator diagnosis " << seq << ", " << count << std::ends;
            return std::string( oss.str() );
        }

    };    // end of class SeqBase<T>::iterator

    iterator begin()  { return iterator( this, 0 ); }
    iterator end()    { return iterator( this, (int)length() ); }

    class const_iterator
    : public random_access_iterator_parent( const Object )
    {
    protected:
        friend class SeqBase<T>;
        const SeqBase<T> *seq;
        sequence_index_type count;

    public:
        const_iterator()                                    : seq( 0 )         , count( 0 )           { }
        const_iterator( const SeqBase<T> *s, int where )    : seq( s )         , count( where )       { }
        const_iterator( const const_iterator& other )       : seq( other.seq ) , count( other.count ) { }
                    ~const_iterator()                                                                             { }

        const T operator*()                         const { return seq->getItem( count ); }
        const T operator[]( sequence_index_type i ) const { return seq->getItem( count + i ); }

        const_iterator& operator=( const const_iterator& other ) {
            if( this !=& other ) {
                seq = other.seq;
                count = other.count;
            }
            return *this;
        }

        bool eql( const const_iterator& other ) const  { return seq->ptr() == other.seq->ptr() && count == other.count; }
        bool neq( const const_iterator& other ) const  { return seq->ptr() != other.seq->ptr() || count != other.count; }
        bool lss( const const_iterator& other ) const  { return                                   count <  other.count; }
        bool gtr( const const_iterator& other ) const  { return                                   count >  other.count; }
        bool leq( const const_iterator& other ) const  { return                                   count <= other.count; }
        bool geq( const const_iterator& other ) const  { return                                   count >= other.count; }

        const_iterator  operator +  ( int n )   const  { return const_iterator( seq, count + n ); }
        const_iterator  operator -  ( int n )   const  { return const_iterator( seq, count - n ); }

        const_iterator& operator += ( int n )          { count += n; return *this; }
        const_iterator& operator -= ( int n )          { count -= n; return *this; }

        int operator-( const const_iterator& other ) const {
            if( *seq != *other.seq )
                throw RuntimeError( "SeqBase<T>::const_iterator::- error" );
            return count - other.count;
        }

        // prefix / postfix
        const_iterator& operator++() { count++; return *this; }   const_iterator operator++( int ) { return const_iterator( seq, count++ ); }
        const_iterator& operator--() { count--; return *this; }   const_iterator operator--( int ) { return const_iterator( seq, count-- ); }

    };    // end of class SeqBase<T>::const_iterator

    const_iterator begin() const  { return const_iterator( this, 0 ); }
    const_iterator end()   const  { return const_iterator( this, length() ); }
};

And here's my revised version:

template<TEMPLATE_TYPENAME T>
class SeqBase: public Object
{
public:
    // :
    // UNTOUCHED
    // :

    template<class I>
    class iterator_base
        : public std::iterator<std::random_access_iterator_tag, seqref<T>, int>
    {
    protected:
        friend class SeqBase<T>; // allow SeqBase<T> to see our data
        SeqBase<T> *seq;
        sequence_index_type count;

        // protected constructors ensure base class is only used as abstract
        iterator_base()                             : seq{ 0 }         , count{ 0 }            { }
        iterator_base( SeqBase<T> *s, int where )   : seq{ s }         , count{ where }        { }
        iterator_base( const iterator_base& other ) : seq{ other.seq } , count{ other.count }  { }

        //virtual ~iterator_base() = 0;
        //{ };

    public:
        I& operator=( const I& other ) {
            if( this != &other ) {
                seq = other.seq;
                count = other.count;
            }
            return  static_cast<I&>( *this );
        }

        bool eql( const I& other ) const  { return seq->ptr() == other.seq->ptr() && count == other.count;  }
        bool neq( const I& other ) const  { return seq->ptr() != other.seq->ptr() || count != other.count;  }
        bool lss( const I& other ) const  { return                                   count <  other.count;  }
        bool gtr( const I& other ) const  { return                                   count >  other.count;  }
        bool leq( const I& other ) const  { return                                   count <= other.count;  }
        bool geq( const I& other ) const  { return                                   count >= other.count;  }

        I  operator +  ( int n )     const  { return I{ seq, count + n }; }
        I  operator -  ( int n )     const  { return I{ seq, count - n }; }
        I& operator += ( int n )            { count += n; return  static_cast<I&>( *this ); }
        I& operator -= ( int n )            { count -= n; return  static_cast<I&>( *this ); }

        int operator-( const I& other ) const {
            if( seq->ptr() != other.seq->ptr() )
                throw RuntimeError{ "SeqBase<T>::iterator comparison error" };
            return count - other.count;
        }

        // prefix / postfix
        I& operator++() { count++; return static_cast<I&>( *this ); }   I operator++( int ) { return I{ seq, count++ }; }
        I& operator--() { count--; return static_cast<I&>( *this ); }   I operator--( int ) { return I{ seq, count-- }; }

        //virtual void make_polymorphic() = 0;

        std::string diagnose() const
        {
            std::OSTRSTREAM oss;
            oss << "iterator diagnosis " << seq << ", " << count << std::ends;
            return std::string( oss.str() );
        }

    };    // end of class SeqBase<T>::iterator


    class iterator : public iterator_base<iterator>
    {
    public:
        iterator()                                      : iterator_base<iterator>{}            { }
        iterator( SeqBase<T> *s, int where )            : iterator_base<iterator>{s, where}    { }
        iterator( const iterator& other )               : iterator_base<iterator>{other}       { }

        // need this->foo, Base<T>::foo or this->Base<T>::foo to access members of base class
        seqref<T> operator*()                           { return seqref<T>{ *(this->seq), this->count };  } // the value this iterator is pointing to
        seqref<T> operator[]( sequence_index_type i )   { return seqref<T>{ *(this->seq), this->count + i };  }

        //void make_polymorphic() override { } // make concrete
        //~iterator() override{ };
    };

    iterator begin()  { return iterator{ this, 0 }; }
    iterator end()    { return iterator{ this, (int)length() }; }

    class const_iterator : public iterator_base<const_iterator>
    {
    public:
        const_iterator()                                : iterator_base<iterator>{}            { }
        const_iterator( SeqBase<T> *s, int where )      : iterator_base<iterator>{s, where}    { }
        const_iterator( const const_iterator& other )   : iterator_base<iterator>{other}       { }

        const T operator*()                         const { return (this->seq)->getItem( this->count ); }
        const T operator[]( sequence_index_type i ) const { return (this->seq)->getItem( this->count + i ); }

        //void make_polymorphic() override { } // make concrete
        //~const_iterator() override{ };
    };

    const_iterator begin() const  { return const_iterator{ this, 0 }; }
    const_iterator end()   const  { return const_iterator{ this, length() }; }
};

For reference, the section I didn't modify, marked as UNTOUCHED, is as follows:

    // STL definitions !!!ToDo: using size_type=size_t etc...
    using size_type         = size_t;
    using value_type        = T;        // TMM: 26Jun'01
    using const_reference   = T;
    using reference         = seqref<T>;
    using pointer           = seqref<T>*;
    using difference_type   = int;

    explicit SeqBase<T>( )                                  : Object{ PyTuple_New(0), true }  { validate(); }
    explicit SeqBase<T>( PyObject* pyob, bool owned=false ) : Object{ pyob,           owned } { validate(); }
             SeqBase<T>( const Object& ob )                 : Object{ ob }                    { validate(); }

    // Assignment acquires new ownership of pointer
    SeqBase<T>& operator=( const Object& rhs )                            {                             return *this = *rhs; }
    SeqBase<T>& operator=( PyObject* rhsp )                               { if(ptr()!=rhsp) set(rhsp);  return *this; }

    bool    accepts( PyObject* pyob )                      const override { return pyob && PySequence_Check(pyob); } // <-- so SeqBase exposes/wraps a Python SEQUENCE object

    virtual size_type size()                                        const { return PySequence_Length( ptr() ); }
            size_type length()                                      const { return PySequence_Length( ptr() ); }

    virtual size_type max_size()                                    const { return std::string::npos; } // ?
    virtual size_type capacity()                                    const { return size(); }

    virtual void swap( SeqBase<T>& c ) { SeqBase<T> temp = c;  c = ptr();  set( temp.ptr() ); }

   // π??? Object has: Object getItem( const Object& key ) const  { return Object( PyObject_GetItem( p, *key ), true ); }

    virtual T    getItem( sequence_index_type index )               const { return T( asObject( PySequence_GetItem(ptr(),index) ) ); }
    virtual void setItem( sequence_index_type index, const T& ob )        {                 if( PySequence_SetItem(ptr(),index,*ob) == -1 ) throw Exception{}; }

    // Element access
    const T operator[]( sequence_index_type index )                 const { return getItem( index ); }
    const T front()                                                 const { return getItem( 0 ); }
    const T back()                                                  const { return getItem( size() - 1 ); }

    seqref<T> operator[]( sequence_index_type index )                     { return seqref<T>{ *this, index          }; }
    seqref<T> front()                                                     { return seqref<T>{ *this, 0              }; }
    seqref<T> back()                                                      { return seqref<T>{ *this, (int)size()-1  }; }

    SeqBase<T> repeat( int count )                                  const { return SeqBase<T>{ PySequence_Repeat( ptr(), count ) , true }; }
    SeqBase<T> concat( const SeqBase<T>& other )                    const { return SeqBase<T>{ PySequence_Concat( ptr(), *other ), true }; }

    // more STL compatability
    void verify_length( size_type required_size )                   const { verify_length( required_size, required_size ); }
    void verify_length( size_type min_size, size_type max_size )    const {
        size_type n = size();
        if( n < min_size || n > max_size )
            throw IndexError{ "Unexpected SeqBase<T> length." };
    }

One thing I not happy about it is the fact that my iterator_base class should maybe been marked as abstract, but I can't see any clean way of doing it.

\$\endgroup\$
1
\$\begingroup\$

From the original Code:

    // prefix / postfix
    iterator& operator++() { count++; return *this; }   iterator operator++( int ) { return iterator( seq, count++ ); }
    iterator& operator--() { count--; return *this; }   iterator operator--( int ) { return iterator( seq, count-- ); }

That's really badly formatted.
Lets give each function its own line:

    // prefix
    iterator& operator++()     { count++; return *this; }
    iterator& operator--()     { count--; return *this; }

    // postfix
    iterator operator++( int ) { return iterator( seq, count++ ); }
    iterator operator--( int ) { return iterator( seq, count-- ); }

Now that I can see them:

    // postfix
    iterator operator++( int ) { return iterator( seq, count++ ); }
    iterator operator--( int ) { return iterator( seq, count-- ); }

This looks so wrong (its actually correct). But I had to take 10 minutes to convince myself that the orginal author got it correct. So I am betting each new maintainer will also need to spend that 10 minutes before he gets it. This does not lead to maintainable code as sombody down the rode is going to come to the wrong conclusion and try and fix it. There solution may or may not work and may break code.

    // postfix
    /*
     * Note: Postfix increments the current iterator
     *       but returns a value that acts like the original iterator before
     *       it was incremented.
     */
    iterator operator++( int ) { iterator result(*this); operator++(); return result; }
    iterator operator--( int ) { iterator result(*this); operator--(); return result; }

Deprecated Code:

This code was obviously writte to conform to the original std::strstream (rather than the more modern std::stringstream). You can tell by the use of std::ends.

    std::string diagnose() const
    {
        std::OSTRSTREAM oss;
        oss << "iterator diagnosis " << seq << ", " << count << std::ends;
        return std::string( oss.str() );
    }

The original version is deprecated so use std::stringstream rather than std::OSTRSTREAM and you can remove the std::ends. The return value can also be simplified.

    std::string diagnose() const
    {
        std::stringstream   oss;
        oss << "iterator diagnosis " << seq << ", " << count;
        return oss.str();
    }

From your code

I would not subclass iterator_base. I would redefine it slightly but then use typedefs to get the two iterator types.

template<typename C>
class iterator_base
{
  private:        // Protected is terrible use it only as a last resort.
    C*  seq;      // Private is much better.
    int count;

  public:
    // protected constructors ensure base class is only used as abstract
    iterator_base()                             : seq{ 0 }         , count{ 0 }            { }
    iterator_base(C* s, int where )             : seq{ s }         , count{ where }        { }
    iterator_base(const iterator_base& other )  : seq{ other.seq } , count{ other.count }  { }

   /// STUFF
};

Then typedef the two iterators:

 typedef iterator_base<SeqBase<T>>           iterator;
 typedef iterator_base<SeqBase<T> const>     const_iterator;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.