Re: [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf)

["François Dumont" <frs.dumont@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4365 bytes --]

On 03/10/2017 22:39, Petr Ovtchenkov wrote:
> On Thu, 28 Sep 2017 13:38:06 +0100
> Jonathan Wakely<jwakely@redhat.com>  wrote:
>
>> On 28/09/17 15:06 +0300, Petr Ovtchenkov wrote:
>>> On Thu, 28 Sep 2017 11:34:25 +0100
>>> Jonathan Wakely<jwakely@redhat.com>  wrote:
>>>>> +  VERIFY(i == std::istreambuf_iterator<char>());
>>>>> +
>>>>> +  VERIFY(memcmp(b, r, 36) == 0);
>>>>> +
>>>>> +  s << q;
>>>>> +  VERIFY(!s.fail());
>>>>> +
>>>>> +  copy_n(i, 36, r);
>>>> This is undefined behaviour. The end-of-stream iterator value cannot
>>>> be dereferenced.
>>> Within this test istreambuf_iterator in eof state never dereferenced.
>> That is quite implementation dependent.
>>
>> The libc++ and VC++ implementations fail this test, because once an
>> istreambuf_iterator has been detected to reach end-of-stream it
>> doesn't get "reset" by changes to the streambuf.
> If we will keep even "unspecified" behaviour same, then bug fix/drawback
> removing become extremely hard: it should be identified as drawback
> in all libs almost simultaneously.
>
>> The libc++ implementation crashes, because operator== on an
>> end-of-stream iterator sets its streambuf* to null, and any further
>> increment or dereference will segfault.
>>
>> So this is testing something that other implementations don't support,
>> and isn't justifiable from the standard.
> I will use N4687 as reference.
>
> 27.2.3 par.2 Table 95:
>
> ++r
>
> Requires: r is dereferenceable. Postconditions: r is dereferenceable or r is
> past-the-end; any copies of the previous value of r are no longer required
> either to be dereferenceable or to be in the domain of ==.
>
> (void)r++ equivalent to (void)++r
>
> *r++
>
> { T tmp = *r;
> ++r;
> return tmp; }
>
> [BTW, you see that r++ without dereference has no sense, and even more,
>
>    copies of the previous
>    value of r are no longer
>    required either to be
>    dereferenceable or to be in
>    the domain of ==.
>
> >From this follow, that postfix increment operator shouldn't return
> istreambuf_iterator.
> ]
>
>>> The test itself simulate "stop and go" istream usage.
>>> stringstream is convenient for behaviuor illustration, but in "real life"
>>> I can assume socket or tty on this place.
>> At the very minimum we should have a comment in the test explaining
>> how it relies on non-standard, non-portable behaviour.
>>
>> But I'd prefer to avoid introducing more divergence from other
>> implementations.
> Standard itself say nothting about "stop and go" scenario.
> At least I don't see any words pro or contra.
> But current implementation block usage of istreambuf_iterator
> with underlying streams like socket or tty, so istreambuf_iterator become
> almost useless structure for practice.
Why not creating a new istreambuf_iterator each time you need to check 
that streambuf is not in eof anymore ?

> We have three issues with istreambuf_iterator:
>    - debug-dependent behaviour
Fixed.
>    - EOL of istreambuf_iterator when it reach EOF (but this not mean
>      EOL of associated streambuf)
Controversial.
>    - postfix increment operator return istreambuf_iterator, but here
>      expected restricted type, that accept only dereference, if it possible.
I agree that we need to fix this last point too.

Consider this code:

   std::istringstream inf("abc");
   std::istreambuf_iterator<char> j(inf), eof;
   std::istreambuf_iterator<char> i = j++;

   assert( *i == 'a' );

At this point it looks like i is pointing to 'a' but then when you do:

std::string str(i, eof);

you have:
assert( str == "ac" );

We jump other the 'b'.

We could improve the situation by adding a debug assertion that _M_c is 
eof when pre-increment is being used or by changing semantic of 
pre-increment to only call sbumpc if _M_c is eof. But then we would need 
to consider _M_c in find overload and in some other places in the lib I 
think.

Rather than going through this complicated path I agree with Petr that 
we need to simply implement the solution advised by the Standard with 
the nested proxy type.

This is what I have done in the attached patch in a naive way. Do we 
need to have abi compatibility here ? If so I'll rework it.

This patch will make libstdc++ pass the llvm test. I even duplicate it 
on our side with a small refinement to check for the return value of the 
proxy::operator*().

François


[-- Attachment #2: istreambuf_iterator_proxy.patch --]
[-- Type: text/x-patch, Size: 4243 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 64b8cfd..a556fce 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,12 +95,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
       mutable streambuf_type*	_M_sbuf;
-      int_type			_M_c;
 
     public:
       ///  Construct end of input stream iterator.
       _GLIBCXX_CONSTEXPR istreambuf_iterator() _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(0), _M_c(traits_type::eof()) { }
+      : _M_sbuf() { }
 
 #if __cplusplus >= 201103L
       istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
@@ -110,11 +109,33 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       ///  Construct start of input stream iterator.
       istreambuf_iterator(istream_type& __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s.rdbuf()) { }
 
       ///  Construct start of streambuf iterator.
       istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s) { }
+
+      class proxy
+      {
+	friend class istreambuf_iterator;
+
+	proxy(streambuf_type* __buf, int_type __c)
+	: _M_sbuf(__buf), _M_c(__c)
+	{ }
+
+	streambuf_type*	_M_sbuf;
+	int_type	_M_c;
+
+      public:
+	char_type
+	operator*() const
+	{ return traits_type::to_char_type(_M_c); }
+      };
+
+      /// Construct start of streambuf iterator.
+      istreambuf_iterator(const proxy& __prx) _GLIBCXX_USE_NOEXCEPT
+      : _M_sbuf(__prx._M_sbuf)
+      { }
 
       ///  Return the current character pointed to by iterator.  This returns
       ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
@@ -138,29 +159,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	_M_sbuf->sbumpc();
-	_M_c = traits_type::eof();
 	return *this;
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
-      istreambuf_iterator
+      proxy
       operator++(int)
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
-	istreambuf_iterator __old = *this;
-	__old._M_c = _M_sbuf->sbumpc();
-	_M_c = traits_type::eof();
-	return __old;
+	return proxy(_M_sbuf, _M_sbuf->sbumpc());
       }
 
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
@@ -175,8 +190,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       int_type
       _M_get() const
       {
-	int_type __ret = _M_c;
-	if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
+	int_type __ret = traits_type::eof();
+	if (_M_sbuf && _S_is_eof(__ret = _M_sbuf->sgetc()))
 	  _M_sbuf = 0;
 	return __ret;
       }
@@ -390,8 +405,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	      else
 		__c = __sb->snextc();
 	    }
-
-	  __first._M_c = __eof;
 	}
 
       return __first;
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
index 572ea9f..de4b8e4 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -23,7 +23,18 @@
 #include <iterator>
 #include <testsuite_hooks.h>
 
-void test02(void)
+void test01()
+{
+  std::istringstream inf("abc");
+  std::istreambuf_iterator<char> j(inf);
+  std::istreambuf_iterator<char> i = j++;
+
+  VERIFY( i != std::istreambuf_iterator<char>() );
+  VERIFY( *i == 'b' );
+  VERIFY( *j++ == 'b' );
+}
+
+void test02()
 {
   typedef std::istreambuf_iterator<char> cistreambuf_iter;
   const char slit01[] = "playa hermosa, liberia, guanacaste";
@@ -111,6 +122,7 @@ void test02(void)
 
 int main()
 {
+  test01();
   test02();
   return 0;
 }