Re: [Boost-users] [boost-users] istream_iterator question - need Steven's help

Subject: Re: [Boost-users] [boost-users] istream_iterator question - need Steven's help
From: Max (more4less_at_[hidden])
Date: 2009-05-16 08:18:10

Please ignore the previous post due to some typo may lead
to misunderstanding. Sorry for the noise.

Hello,

I hope I could get the attention and, hopefully, help from the guru's 
like Steven Watanabe, et al.  :-)

After careful analysis of the source code, I get the conclusion:

The absence of 'if(n)' is not the beef of the problem. On the contrary, 
even though it could make the code bahave as expected, it will break
other code using ordinary iterators (that means iterators other than 
istream_iterator).

Original version (without 'if(n)') of both copy_n are logically correct, 
which have been domenstrated. The design of istream_iterator makes 
its behavior generally as same as an iterator, but will break the simple 
test code in my OP. I think this is a design flaw.

The definition of istream_iterator that comes with VS2008 is
as follows (simplified and unrelated part omitted):

template<class _Ty,
        class _Elem = char,
        class _Traits = char_traits<_Elem>,
        class _Diff = ptrdiff_t>
        class istream_iterator
                : public iterator<input_iterator_tag, _Ty, _Diff,
                        const _Ty *, const _Ty&>
        {	// wrap _Ty extracts from input stream as input iterator
        typedef istream_iterator<_Ty, _Elem, _Traits, _Diff> _Myt;
public:
        typedef _Elem char_type;
        typedef _Traits traits_type;
        typedef basic_istream<_Elem, _Traits> istream_type;

        istream_iterator(istream_type& _Istr)
                : _Myistr(&_Istr)
                {	// construct with input stream
                _Getval();
                }

        const _Ty& operator*() const
                {	// return designated value
                return (_Myval);
                }

        const _Ty *operator->() const
                {	// return pointer to class object
                return (&**this);
                }

        _Myt& operator++()
                {	// preincrement
                _Getval();
                return (*this);
                }

        _Myt operator++(int)
                {	// postincrement
                _Myt _Tmp = *this;
                ++*this;
                return (_Tmp);
                }

protected:
        void _Getval()
                {	// get a _Ty value if possible
                if (_Myistr != 0 && !(*_Myistr >> _Myval))
                        _Myistr = 0;
                }

        istream_type *_Myistr;	// pointer to input stream
        _Ty _Myval;	// lookahead value (valid if _Myistr is not null)
        };

IMO, it should be modified as:

template<class _Ty,
        class _Elem = char,
        class _Traits = char_traits<_Elem>,
        class _Diff = ptrdiff_t>
        class istream_iterator
                : public iterator<input_iterator_tag, _Ty, _Diff,
                        const _Ty *, const _Ty&>
        {	// wrap _Ty extracts from input stream as input iterator
        typedef istream_iterator<_Ty, _Elem, _Traits, _Diff> _Myt;
public:
        typedef _Elem char_type;
        typedef _Traits traits_type;
        typedef basic_istream<_Elem, _Traits> istream_type;

        istream_iterator(istream_type& _Istr)
                : _Myistr(&_Istr)
                {	// construct with input stream
                }

        const _Ty& operator*() //const
                {	// return designated value
                _Getval();
                return (_Myval);
                }

        const _Ty *operator->() //const
                {	// return pointer to class object
                return (&**this);
                }

        _Myt& operator++()
                {	// preincrement
                return (*this);
                }

        _Myt operator++(int)
                {	// postincrement
                return (*this);
                }

protected:
        void _Getval()
                {	// get a _Ty value if possible
                if (_Myistr != 0 && !(*_Myistr >> _Myval))
                        _Myistr = 0;
                }

        istream_type *_Myistr;	// pointer to input stream
        _Ty _Myval;	// to-be-read value (valid if _Myistr is not null)
        };

The main problem of the current design of istream_iterator is that
it always swallow one extra object, of the template parameter
type, prepared for the coming * or -> operator. This makes the 
coming plain >> operator a distance forward of where it shoud be.

I've tested with this modification and found it works well. Please
note that I've simply commented out the trailing const qualifier 
for the * and -> operator, to make things simpler and more 
straightforward. Obviously, furthur modification is to be made to 
make the code to a product level.

I want to know whether this change makes sense?

B/Rgds
Max

> > Hello,
> >
> > I don't know why the following code does not work as expected.
> > I hope the guru's here could lend me a hand.
> >
> > For a test data file:
> > 3
> > 1 2
> > 3 4
> > 5 6
> >
> > 2
> > 1.2 4.3 234.2
> > 3.5656 67.88 345.3
> > <eof>
> >
> > and 2 simple test helper class:
> >
> > struct a
> > { int i, j; };
> > std::istream& operator>>(std::istream& in, a & p)
> > {  in >> p.i >> p.j;  return in;  }
> > std::ostream& operator<<(std::ostream& out, const a & p)
> > {  out << p.i << " " << p.j; return out; }
> >
> > struct b
> > { float i, j, k; };
> > std::istream& operator>>(std::istream& in, b & p)
> > {  in >> p.i >> p.j >> p.k; return in;  }
> > std::ostream& operator<<(std::ostream& out, const b & p)
> > {  out << p.i << " " << p.j << " " << p.k; return out;  }
> [Snip]
> >
> > copy_n is defined as:
> >
> > template <class InputIterator, class Size, class OutputIterator>
> > 	OutputIterator copy_n( InputIterator first, InputIterator last,
> > 	Size n,
> > 	OutputIterator result)
> > {
> > 	// copies the first `n' items from `first' to `result'. Returns
> > 	// the value of `result' after inserting the `n' items.
> > 	// it ends before n iterations, if the last iterator is reached
> > 	while( n-- && first != last)
> > 	{
> > 		*result = *first;
> > 		++first;
> > 		++result;
> > 	}
> > 	return result;
> > }
> 
> Change your copy_n() as follows:
> 
> template <class InputIterator, class Size, class OutputIterator>
> 	OutputIterator copy_n( InputIterator first, InputIterator last,
> 	Size n,
> 	OutputIterator result)
> {
> 	// copies the first `n' items from `first' to `result'. Returns
> 	// the value of `result' after inserting the `n' items.
> 	// it ends before n iterations, if the last iterator is reached
> 	while( n-- && first != last)
> 	{
> 		*result = *first;
> 		if (n) ++first;
> 		++result;
> 	}
> 	return result;
> }
> 
> Regards,
> Dmitry

Hello Dmitry,

I really appreciate your help on such a naive question! 
I'm prepared to getting no response from the list, for my 
message is really a little bit lengthy, and somewhat off 
the main stream of the list, I'm afraid. :-)

With your code change, the test code runs as expected,
so does my real program. But I just don't know why.

In fact, the copy_n function is a safe version of another
one:

template <class InputIterator, class Size, class OutputIterator>
        OutputIterator copy_n( InputIterator first,
        Size n,
        OutputIterator result) 
{
        // copies the first `n' items from `first' to `result'. Returns
        // the value of `result' after inserting the `n' items.
        while( n-- ) 
        {
                *result = *first;
                ++first;
                ++result;
        }
        return result;
}

I cannot find bug from it, and I think the safer one is also 
straightforward and correct. :-)

I think 'first' and 'result' should both increment with 1 if needed
to keep sync. With your extra 'if(n)' condition they will be out of sync.
But that is just working well. I'm really confused.

Thanks again for your help.

B/Rgds
Max