• 35.2k
• 13
• 134
• 238

• 639
• 5
• 13

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.