[boost-users] istream_iterator question
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; } Here is version 1, quite trivial and works just ok. std::ifstream is("./test.txt") int n; is >> n; std::cout << "n=" << n << "\n"; for(int i=0; i<n; i++) { a tmp; is >> tmp; va.push_back(tmp); } std::copy(va.begin(), va.end(), std::ostream_iterator<a>(std::cout, "\n")); is >> n; std::cout << "n=" << n << "\n"; for(int i=0; i<n; i++) { b tmp; is >> tmp; vb.push_back(tmp); } std::copy(vb.begin(), vb.end(), std::ostream_iterator<b>(std::cout, "\n")); The output is: n=3 1 2 3 4 5 6 n=2 1.2 4.3 234.2 3.5656 67.88 345.3 With version 2 using istream_iterator, the behavior is strange: is >> n; std::cout << "n=" << n << "\n"; std::copy_n(std::istream_iterator<a>(is), std::istream_iterator<a>(), n, std::back_inserter(va)); std::copy(va.begin(), va.end(), std::ostream_iterator<a>(std::cout, "\n")); is >> n; std::cout << "n=" << n << "\n"; std::copy_n(std::istream_iterator<b>(is), std::istream_iterator<b>(), n, std::back_inserter(vb)); std::copy(vb.begin(), vb.end(), std::ostream_iterator<b>(std::cout, "\n")); 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; } The output is now: n=3 1 2 3 4 5 6 n=3 I've checked the code for several times and just not found the answer. I'm using VS2008SP1 on WinXP/SP1 Thanks for your help. B/Rgds Max
Max escribió:
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
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 desing 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 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
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 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
AMDG Max wrote:
Well, it solves your problem, but it creates other problems instead. For example, what should this do: std::istream_iterator<int> iter(std::cin); std::advance(iter, 10); Or this: std::istream_iterator<int> iter(std::cin); if(*iter != 10) { std::cout << *iter; } In Christ, Steven Watanabe
hello Steven, I do appreciate your prompt reply.
Yes, my simple modification is obviously not complete and will cause other problems including those you mentioned. But I believe we are not at the situation that we should bear one of the two kinds of problems. There must be a sound solution, I guess. Or, in other words, in the situation as that I mentioned in my first post, should I use the ugly-looking explicit for loops instead of using the seemingly elegant iterator adaptor approach? :-) Thanks again for your time, Steven. B/Rgds Max
participants (3)
-
Dmitry Bufistov -
Max -
Steven Watanabe