* [PATCH] streambuf_iterator: avoid debug-dependent behaviour
@ 2017-08-24 11:58 Petr Ovtchenkov
2017-09-01 9:10 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-08-24 11:58 UTC (permalink / raw)
To: libstdc++; +Cc: gcc-patches
Explicit do sgetc from associated streambuf. Avoid debug-dependent
sgetc (within _M_at_eof()):
__glibcxx_requires_cond(!_M_at_eof(),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
Increment operators not require not-eof precoditions.
Don't unlink associated streambuf if eof detected (in _M_get).
Clean logic in postfix increment operator.
---
libstdc++-v3/include/bits/streambuf_iterator.h | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 2230e94..cff3b69 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -136,9 +136,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
- _M_message(__gnu_debug::__msg_inc_istreambuf)
- ._M_iterator(*this));
if (_M_sbuf)
{
_M_sbuf->sbumpc();
@@ -151,14 +148,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
- _M_message(__gnu_debug::__msg_inc_istreambuf)
- ._M_iterator(*this));
+ _M_get();
istreambuf_iterator __old = *this;
if (_M_sbuf)
{
- __old._M_c = _M_sbuf->sbumpc();
+ _M_sbuf->sbumpc();
_M_c = traits_type::eof();
}
return __old;
@@ -177,18 +172,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_get() const
{
const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
- if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
- _M_sbuf = 0;
- }
- return __ret;
+ if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
+ _M_c = _M_sbuf->sgetc();
+ return _M_c;
}
bool
--
2.10.1
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] streambuf_iterator: avoid debug-dependent behaviour
2017-08-24 11:58 [PATCH] streambuf_iterator: avoid debug-dependent behaviour Petr Ovtchenkov
@ 2017-09-01 9:10 ` Jonathan Wakely
2017-09-07 21:02 ` François Dumont
0 siblings, 1 reply; 55+ messages in thread
From: Jonathan Wakely @ 2017-09-01 9:10 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
On 24/08/17 12:57 +0300, Petr Ovtchenkov wrote:
>Explicit do sgetc from associated streambuf. Avoid debug-dependent
>sgetc (within _M_at_eof()):
>
> __glibcxx_requires_cond(!_M_at_eof(),
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
>
>Increment operators not require not-eof precoditions.
>
>Don't unlink associated streambuf if eof detected (in _M_get).
>
>Clean logic in postfix increment operator.
I find it very hard to understand the reasons for this patch. What
you've written above is too terse for me.
Are you fixing a bug? If so, do you have a testcase that demonstrates
the problem, and is fixed by these changes?
Is this just refactoring, without changing behaviour?
Finally, and very importantly, do you have a copyright assignment for
changes to GCC? See https://gcc.gnu.org/contribute.html#legal
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] streambuf_iterator: avoid debug-dependent behaviour
2017-09-01 9:10 ` Jonathan Wakely
@ 2017-09-07 21:02 ` François Dumont
2017-09-08 5:47 ` Petr Ovtchenkov
0 siblings, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-09-07 21:02 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2218 bytes --]
Hi
Following this report I had a closer look to this debug-dependent
behavior which I agree about.
I have added some checks to 2.cc as shown in the patch. Initially the
test was failing because:
/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc:45:
void test02(): Assertion 'old != istrb_eos' failed.
This was coming from the __old._M_c = _M_sbuf->sbumpc() line in the
post-increment operator. The post-increment is supposed to return a copy
of the current iterator which this assignment is compromising. So I try
to remove the assignment and test failed later:
/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc:104:
void test02(): Assertion 'c == slit01[i]' failed.
However, run in debug mode, it worked fine.
The test is failing in normal mode because the iterator instantiation
doesn't read the streambuf and without the previous __old._M_c
assignment _M_c is still pointing to eos. In debug mode, _M_at_eof()
initialize the iterator and so it work nicely.
So for those reasons I would like to propose attached patch.
Additionally it gets rid of the mutable usage. And I also remove a
useless noexcept qualification on the defaulted copy constructor.
François
On 01/09/2017 11:10, Jonathan Wakely wrote:
> On 24/08/17 12:57 +0300, Petr Ovtchenkov wrote:
>> Explicit do sgetc from associated streambuf. Avoid debug-dependent
>> sgetc (within _M_at_eof()):
>>
>> __glibcxx_requires_cond(!_M_at_eof(),
>> _M_message(__gnu_debug::__msg_inc_istreambuf)
>> ._M_iterator(*this));
>>
>> Increment operators not require not-eof precoditions.
>>
>> Don't unlink associated streambuf if eof detected (in _M_get).
>>
>> Clean logic in postfix increment operator.
>
> I find it very hard to understand the reasons for this patch. What
> you've written above is too terse for me.
>
> Are you fixing a bug? If so, do you have a testcase that demonstrates
> the problem, and is fixed by these changes?
>
> Is this just refactoring, without changing behaviour?
>
> Finally, and very importantly, do you have a copyright assignment for
> changes to GCC? See https://gcc.gnu.org/contribute.html#legal
>
>
>
[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5166 bytes --]
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..27d0725 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,8 +94,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// the "end of stream" iterator value.
// NB: This implementation assumes the "end of stream" value
// is EOF, or -1.
- mutable streambuf_type* _M_sbuf;
- mutable int_type _M_c;
+ streambuf_type* _M_sbuf;
+ int_type _M_c;
public:
/// Construct end of input stream iterator.
@@ -103,18 +103,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
: _M_sbuf(0), _M_c(traits_type::eof()) { }
#if __cplusplus >= 201103L
- istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+ istreambuf_iterator(const istreambuf_iterator&) = default;
~istreambuf_iterator() = default;
#endif
/// 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()), _M_c(traits_type::eof())
+ {
+ if (_M_sbuf)
+ _M_get();
+ }
/// Construct start of streambuf iterator.
istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
- : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+ : _M_sbuf(__s), _M_c(traits_type::eof())
+ {
+ if (_M_sbuf)
+ _M_get();
+ }
/// Return the current character pointed to by iterator. This returns
/// streambuf.sgetc(). It cannot be assigned. NB: The result of
@@ -122,28 +130,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
char_type
operator*() const
{
+ int_type __c = _M_get();
+
#ifdef _GLIBCXX_DEBUG_PEDANTIC
// Dereferencing a past-the-end istreambuf_iterator is a
// libstdc++ extension
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(!_S_at_eof(__c),
_M_message(__gnu_debug::__msg_deref_istreambuf)
._M_iterator(*this));
#endif
- return traits_type::to_char_type(_M_get());
+ return traits_type::to_char_type(__c);
}
/// Advance the iterator. Calls streambuf.sbumpc().
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf,
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
- if (_M_sbuf)
- {
- _M_sbuf->sbumpc();
- _M_c = traits_type::eof();
- }
+ _M_sbuf->sbumpc();
+ _M_get();
return *this;
}
@@ -151,16 +158,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
- _M_message(__gnu_debug::__msg_inc_istreambuf)
- ._M_iterator(*this));
-
istreambuf_iterator __old = *this;
- if (_M_sbuf)
- {
- __old._M_c = _M_sbuf->sbumpc();
- _M_c = traits_type::eof();
- }
+ ++(*this);
return __old;
}
@@ -176,26 +175,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
int_type
_M_get() const
{
- const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
+ if (!_S_at_eof(_M_c))
+ return _M_c;
+
if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
- _M_sbuf = 0;
- }
- return __ret;
+ return _M_sbuf->sgetc();
+ return _M_c;
+ }
+
+ void
+ _M_get()
+ {
+ _M_c = _M_sbuf->sgetc();
+ if (_S_at_eof(_M_c))
+ _M_sbuf = 0;
}
bool
_M_at_eof() const
+ { return _S_at_eof(_M_get()); }
+
+ static bool
+ _S_at_eof(int_type __c)
{
const int_type __eof = traits_type::eof();
- return traits_type::eq_int_type(_M_get(), __eof);
+ return traits_type::eq_int_type(__c, __eof);
}
};
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
index b81f4d4..498dc1b 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
void test02(void)
{
-
typedef std::istreambuf_iterator<char> cistreambuf_iter;
- typedef cistreambuf_iter::streambuf_type cstreambuf_type;
const char slit01[] = "playa hermosa, liberia, guanacaste";
std::string str01(slit01);
std::istringstream istrs00(str01);
@@ -35,10 +33,18 @@ void test02(void)
// ctor sanity checks
cistreambuf_iter istrb_it01(istrs00);
- cistreambuf_iter istrb_it02;
- std::string tmp(istrb_it01, istrb_it02);
+ cistreambuf_iter istrb_eos;
+ VERIFY( istrb_it01 != istrb_eos );
+
+ std::string tmp(istrb_it01, istrb_eos);
VERIFY( tmp == str01 );
+ VERIFY( istrb_it01 != istrb_eos );
+ VERIFY( *istrb_it01 == 'p' );
+ cistreambuf_iter old = istrb_it01++;
+ VERIFY( old != istrb_eos );
+ VERIFY( istrb_it01 == istrb_eos );
+
cistreambuf_iter istrb_it03(0);
cistreambuf_iter istrb_it04;
VERIFY( istrb_it03 == istrb_it04 );
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] streambuf_iterator: avoid debug-dependent behaviour
2017-09-07 21:02 ` François Dumont
@ 2017-09-08 5:47 ` Petr Ovtchenkov
2017-09-09 20:17 ` François Dumont
0 siblings, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-09-08 5:47 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
-gcc-patches
On Thu, 7 Sep 2017 23:02:15 +0200
François Dumont <frs.dumont@gmail.com> wrote:
> + _M_c = _M_sbuf->sgetc();
> + if (_S_at_eof(_M_c))
> + _M_sbuf = 0;
_M_sbuf = 0; <--- Is not what I axpect here.
Suggestions will be later, after we finish copyright assignment for
changes procedure (in progress).
WBR,
--
- ptr
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] streambuf_iterator: avoid debug-dependent behaviour
2017-09-08 5:47 ` Petr Ovtchenkov
@ 2017-09-09 20:17 ` François Dumont
2017-09-21 5:46 ` François Dumont
0 siblings, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-09-09 20:17 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]
Hi
Completing the execution of tests revealed a lot about the current
implementation.
The main point of current implementation is to delay as much as
possible the capture of the current streambuf position. So my original
proposal capturing state on instantiation was wrong.
This new proposal concentrate on the debug-dependent code. Debug
assertions now avoids calling _M_at_eof() which also capture iterator
state. It also simplifies _M_get() method a little bit like Petr
proposed but keeping the _M_sbuf reset when reaching eof. Thanks to this
work I realized that std::find specialization could also be simplified
by returning a streambuf_iterator which will capture current streambuf
state on evaluation.
Note that I haven't been able to create a test case revealing the
problem. This is more a code quality issue as current code violates the
principal that debug asserts shouldn't impact object state. AFAIK this
is noticeable only under gdb.
Regarding avoiding the reset of _M_sbuf it might be possible,your
test case could be a good reason to do so. But this is going to be a big
change for current implementation so don't forget to run all testsuite
and to consider the std::copy and std::find specializations.
Tested under Linux x86_64, normal and debug modes.
Ok to commit ?
François
On 08/09/2017 07:47, Petr Ovtchenkov wrote:
> -gcc-patches
>
> On Thu, 7 Sep 2017 23:02:15 +0200
> François Dumont <frs.dumont@gmail.com> wrote:
>
>> + _M_c = _M_sbuf->sgetc();
>> + if (_S_at_eof(_M_c))
>> + _M_sbuf = 0;
> _M_sbuf = 0; <--- Is not what I axpect here.
>
> Suggestions will be later, after we finish copyright assignment for
> changes procedure (in progress).
>
> WBR,
>
> --
>
> - ptr
>
[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5021 bytes --]
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..ad9c89f 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
: _M_sbuf(0), _M_c(traits_type::eof()) { }
#if __cplusplus >= 201103L
- istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+ istreambuf_iterator(const istreambuf_iterator&) = default;
~istreambuf_iterator() = default;
#endif
@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
char_type
operator*() const
{
+ int_type __c = _M_get();
+
#ifdef _GLIBCXX_DEBUG_PEDANTIC
// Dereferencing a past-the-end istreambuf_iterator is a
// libstdc++ extension
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(!_S_at_eof(__c),
_M_message(__gnu_debug::__msg_deref_istreambuf)
._M_iterator(*this));
#endif
- return traits_type::to_char_type(_M_get());
+ return traits_type::to_char_type(__c);
}
/// Advance the iterator. Calls streambuf.sbumpc().
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf
+ && (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
- if (_M_sbuf)
- {
+
_M_sbuf->sbumpc();
_M_c = traits_type::eof();
- }
return *this;
}
@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf
+ && (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
istreambuf_iterator __old = *this;
- if (_M_sbuf)
- {
__old._M_c = _M_sbuf->sbumpc();
_M_c = traits_type::eof();
- }
return __old;
}
@@ -176,26 +175,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
int_type
_M_get() const
{
- const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
- if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
+ if (_M_sbuf && _S_at_eof(_M_c) && _S_at_eof(_M_c = _M_sbuf->sgetc()))
_M_sbuf = 0;
- }
- return __ret;
+ return _M_c;
}
bool
_M_at_eof() const
+ { return _S_at_eof(_M_get()); }
+
+ static bool
+ _S_at_eof(int_type __c)
{
const int_type __eof = traits_type::eof();
- return traits_type::eq_int_type(_M_get(), __eof);
+ return traits_type::eq_int_type(__c, __eof);
}
};
@@ -373,13 +366,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::traits_type traits_type;
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
+ const int_type __eof = traits_type::eof();
if (__first._M_sbuf && !__last._M_sbuf)
{
const int_type __ival = traits_type::to_int_type(__val);
streambuf_type* __sb = __first._M_sbuf;
int_type __c = __sb->sgetc();
- while (!traits_type::eq_int_type(__c, traits_type::eof())
+ while (!traits_type::eq_int_type(__c, __eof)
&& !traits_type::eq_int_type(__c, __ival))
{
streamsize __n = __sb->egptr() - __sb->gptr();
@@ -396,11 +390,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__c = __sb->snextc();
}
- if (!traits_type::eq_int_type(__c, traits_type::eof()))
- __first._M_c = __c;
- else
- __first._M_sbuf = 0;
+ __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 b81f4d4..e3d99f9 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
void test02(void)
{
-
typedef std::istreambuf_iterator<char> cistreambuf_iter;
- typedef cistreambuf_iter::streambuf_type cstreambuf_type;
const char slit01[] = "playa hermosa, liberia, guanacaste";
std::string str01(slit01);
std::istringstream istrs00(str01);
@@ -35,10 +33,17 @@ void test02(void)
// ctor sanity checks
cistreambuf_iter istrb_it01(istrs00);
- cistreambuf_iter istrb_it02;
- std::string tmp(istrb_it01, istrb_it02);
+ cistreambuf_iter istrb_eos;
+ VERIFY( istrb_it01 != istrb_eos );
+
+ std::string tmp(istrb_it01, istrb_eos);
VERIFY( tmp == str01 );
+ VERIFY( istrb_it01 != istrb_eos );
+ cistreambuf_iter old = istrb_it01++;
+ VERIFY( old == istrb_eos );
+ VERIFY( istrb_it01 == istrb_eos );
+
cistreambuf_iter istrb_it03(0);
cistreambuf_iter istrb_it04;
VERIFY( istrb_it03 == istrb_it04 );
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] streambuf_iterator: avoid debug-dependent behaviour
2017-09-09 20:17 ` François Dumont
@ 2017-09-21 5:46 ` François Dumont
2017-09-28 10:50 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-09-21 5:46 UTC (permalink / raw)
To: libstdc++; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2725 bytes --]
Gentle reminder, ok to commit ?
* include/bits/streambuf_iterator.h
(istreambuf_iterator<>(const istreambuf_iterator&)): Remove useless
noexcept qualificatio<n on defaulted implementation.
(istreambuf_iterator<>::operator*()): Do not capture iterator state
in Debug assertion.
(istreambuf_iterator<>::operator++()): Likewise.
(istreambuf_iterator<>::operator++(int)): Likewise.
(istreambuf_iterator<>::_M_get()): Simplify implementation.
(istreambuf_iterator<>::_S_at_eof()): New.
(istreambuf_iterator<>::_M_at_eof()): Adapt, use latter.
(find(istreambuf_iterator<>, istreambuf_iterator<>, _CharT)):
Return an iterator with _M_c set to eof to capture streambuf state
on evaluation.
(testsuite/24_iterators/istreambuf_iterator/2.cc): Add checks.
François
On 09/09/2017 22:16, François Dumont wrote:
> Hi
>
> Completing the execution of tests revealed a lot about the current
> implementation.
>
> The main point of current implementation is to delay as much as
> possible the capture of the current streambuf position. So my original
> proposal capturing state on instantiation was wrong.
>
> This new proposal concentrate on the debug-dependent code. Debug
> assertions now avoids calling _M_at_eof() which also capture iterator
> state. It also simplifies _M_get() method a little bit like Petr
> proposed but keeping the _M_sbuf reset when reaching eof. Thanks to
> this work I realized that std::find specialization could also be
> simplified by returning a streambuf_iterator which will capture
> current streambuf state on evaluation.
>
> Note that I haven't been able to create a test case revealing the
> problem. This is more a code quality issue as current code violates
> the principal that debug asserts shouldn't impact object state. AFAIK
> this is noticeable only under gdb.
>
> Regarding avoiding the reset of _M_sbuf it might be possible,your
> test case could be a good reason to do so. But this is going to be a
> big change for current implementation so don't forget to run all
> testsuite and to consider the std::copy and std::find specializations.
>
> Tested under Linux x86_64, normal and debug modes.
>
> Ok to commit ?
>
> François
>
>
> On 08/09/2017 07:47, Petr Ovtchenkov wrote:
>> -gcc-patches
>>
>> On Thu, 7 Sep 2017 23:02:15 +0200
>> François Dumont <frs.dumont@gmail.com> wrote:
>>
>>> + _M_c = _M_sbuf->sgetc();
>>> + if (_S_at_eof(_M_c))
>>> + _M_sbuf = 0;
>> _M_sbuf = 0; <--- Is not what I axpect here.
>>
>> Suggestions will be later, after we finish copyright assignment for
>> changes procedure (in progress).
>>
>> WBR,
>>
>> --
>>
>> - ptr
>>
>
[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5021 bytes --]
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..ad9c89f 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
: _M_sbuf(0), _M_c(traits_type::eof()) { }
#if __cplusplus >= 201103L
- istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+ istreambuf_iterator(const istreambuf_iterator&) = default;
~istreambuf_iterator() = default;
#endif
@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
char_type
operator*() const
{
+ int_type __c = _M_get();
+
#ifdef _GLIBCXX_DEBUG_PEDANTIC
// Dereferencing a past-the-end istreambuf_iterator is a
// libstdc++ extension
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(!_S_at_eof(__c),
_M_message(__gnu_debug::__msg_deref_istreambuf)
._M_iterator(*this));
#endif
- return traits_type::to_char_type(_M_get());
+ return traits_type::to_char_type(__c);
}
/// Advance the iterator. Calls streambuf.sbumpc().
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf
+ && (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
- if (_M_sbuf)
- {
+
_M_sbuf->sbumpc();
_M_c = traits_type::eof();
- }
return *this;
}
@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf
+ && (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
istreambuf_iterator __old = *this;
- if (_M_sbuf)
- {
__old._M_c = _M_sbuf->sbumpc();
_M_c = traits_type::eof();
- }
return __old;
}
@@ -176,26 +175,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
int_type
_M_get() const
{
- const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
- if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
+ if (_M_sbuf && _S_at_eof(_M_c) && _S_at_eof(_M_c = _M_sbuf->sgetc()))
_M_sbuf = 0;
- }
- return __ret;
+ return _M_c;
}
bool
_M_at_eof() const
+ { return _S_at_eof(_M_get()); }
+
+ static bool
+ _S_at_eof(int_type __c)
{
const int_type __eof = traits_type::eof();
- return traits_type::eq_int_type(_M_get(), __eof);
+ return traits_type::eq_int_type(__c, __eof);
}
};
@@ -373,13 +366,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::traits_type traits_type;
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
+ const int_type __eof = traits_type::eof();
if (__first._M_sbuf && !__last._M_sbuf)
{
const int_type __ival = traits_type::to_int_type(__val);
streambuf_type* __sb = __first._M_sbuf;
int_type __c = __sb->sgetc();
- while (!traits_type::eq_int_type(__c, traits_type::eof())
+ while (!traits_type::eq_int_type(__c, __eof)
&& !traits_type::eq_int_type(__c, __ival))
{
streamsize __n = __sb->egptr() - __sb->gptr();
@@ -396,11 +390,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__c = __sb->snextc();
}
- if (!traits_type::eq_int_type(__c, traits_type::eof()))
- __first._M_c = __c;
- else
- __first._M_sbuf = 0;
+ __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 b81f4d4..e3d99f9 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
void test02(void)
{
-
typedef std::istreambuf_iterator<char> cistreambuf_iter;
- typedef cistreambuf_iter::streambuf_type cstreambuf_type;
const char slit01[] = "playa hermosa, liberia, guanacaste";
std::string str01(slit01);
std::istringstream istrs00(str01);
@@ -35,10 +33,17 @@ void test02(void)
// ctor sanity checks
cistreambuf_iter istrb_it01(istrs00);
- cistreambuf_iter istrb_it02;
- std::string tmp(istrb_it01, istrb_it02);
+ cistreambuf_iter istrb_eos;
+ VERIFY( istrb_it01 != istrb_eos );
+
+ std::string tmp(istrb_it01, istrb_eos);
VERIFY( tmp == str01 );
+ VERIFY( istrb_it01 != istrb_eos );
+ cistreambuf_iter old = istrb_it01++;
+ VERIFY( old == istrb_eos );
+ VERIFY( istrb_it01 == istrb_eos );
+
cistreambuf_iter istrb_it03(0);
cistreambuf_iter istrb_it04;
VERIFY( istrb_it03 == istrb_it04 );
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] libstdc++: istreambuf_iterator keep attached streambuf
@ 2017-09-23 7:10 Petr Ovtchenkov
2017-09-25 13:46 ` Jonathan Wakely
2017-09-28 10:34 ` Jonathan Wakely
0 siblings, 2 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-09-23 7:10 UTC (permalink / raw)
To: libstdc++; +Cc: gcc-patches
istreambuf_iterator should not forget about attached
streambuf when it reach EOF.
Checks in debug mode has no infuence more on character
extraction in istreambuf_iterator increment operators.
In this aspect behaviour in debug and non-debug mode
is similar now.
Test for detached srteambuf in istreambuf_iterator:
When istreambuf_iterator reach EOF of istream, it should not
forget about attached streambuf.
From fact "EOF in stream reached" not follow that
stream reach end of life and input operation impossible
more.
---
libstdc++-v3/include/bits/streambuf_iterator.h | 41 +++++++--------
.../24_iterators/istreambuf_iterator/3.cc | 61 ++++++++++++++++++++++
2 files changed, 80 insertions(+), 22 deletions(-)
create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..45c3d89 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf,
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
if (_M_sbuf)
{
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+ int_type _tmp =
+#endif
_M_sbuf->sbumpc();
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+ __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
+ _M_message(__gnu_debug::__msg_inc_istreambuf)
+ ._M_iterator(*this));
+#endif
_M_c = traits_type::eof();
}
return *this;
@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ _M_get();
+ __glibcxx_requires_cond(_M_sbuf
+ && !traits_type::eq_int_type(_M_c,traits_type::eof()),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
istreambuf_iterator __old = *this;
if (_M_sbuf)
{
- __old._M_c = _M_sbuf->sbumpc();
+ _M_sbuf->sbumpc();
_M_c = traits_type::eof();
}
return __old;
@@ -177,18 +187,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_get() const
{
const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
- if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
- _M_sbuf = 0;
- }
- return __ret;
+ if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
+ _M_c = _M_sbuf->sgetc();
+ return _M_c;
}
bool
@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
- if (__first._M_sbuf && !__last._M_sbuf)
+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
{
streambuf_type* __sb = __first._M_sbuf;
int_type __c = __sb->sgetc();
@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
- if (__first._M_sbuf && !__last._M_sbuf)
+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
{
const int_type __ival = traits_type::to_int_type(__val);
streambuf_type* __sb = __first._M_sbuf;
@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
else
__c = __sb->snextc();
}
-
- if (!traits_type::eq_int_type(__c, traits_type::eof()))
- __first._M_c = __c;
- else
- __first._M_sbuf = 0;
+ __first._M_c = __c;
}
return __first;
}
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
new file mode 100644
index 0000000..803ede4
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
@@ -0,0 +1,61 @@
+// { dg-options "-std=gnu++17" }
+
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+#include <cstring>
+#include <testsuite_hooks.h>
+
+void test03()
+{
+ using namespace std;
+ bool test __attribute__((unused)) = true;
+
+ std::stringstream s;
+ char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
+ char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
+ char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
+ // 012345678901234567890123456789012345
+ // 0 1 2 3
+ s << b;
+ VERIFY( !s.fail() );
+
+ istreambuf_iterator<char> i(s);
+ copy_n(i, 36, r);
+ ++i; // EOF reached
+ VERIFY(i == std::istreambuf_iterator<char>());
+
+ VERIFY(memcmp(b, r, 36) == 0);
+
+ s << q;
+ VERIFY(!s.fail());
+
+ copy_n(i, 36, r);
+ ++i; // EOF reached
+ VERIFY(i == std::istreambuf_iterator<char>());
+
+ VERIFY(memcmp(q, r, 36) == 0);
+}
+
+int main()
+{
+ test03();
+ return 0;
+}
--
2.10.1
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf
2017-09-23 7:10 [PATCH] libstdc++: istreambuf_iterator keep attached streambuf Petr Ovtchenkov
@ 2017-09-25 13:46 ` Jonathan Wakely
2017-09-28 10:34 ` Jonathan Wakely
1 sibling, 0 replies; 55+ messages in thread
From: Jonathan Wakely @ 2017-09-25 13:46 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches, François Dumont
On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
>istreambuf_iterator should not forget about attached
>streambuf when it reach EOF.
>
>Checks in debug mode has no infuence more on character
>extraction in istreambuf_iterator increment operators.
>In this aspect behaviour in debug and non-debug mode
>is similar now.
>
>Test for detached srteambuf in istreambuf_iterator:
>When istreambuf_iterator reach EOF of istream, it should not
>forget about attached streambuf.
From fact "EOF in stream reached" not follow that
>stream reach end of life and input operation impossible
>more.
>---
> libstdc++-v3/include/bits/streambuf_iterator.h | 41 +++++++--------
> .../24_iterators/istreambuf_iterator/3.cc | 61 ++++++++++++++++++++++
> 2 files changed, 80 insertions(+), 22 deletions(-)
> create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>
>diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
>index f0451b1..45c3d89 100644
>--- a/libstdc++-v3/include/bits/streambuf_iterator.h
>+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
>@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> istreambuf_iterator&
> operator++()
> {
>- __glibcxx_requires_cond(!_M_at_eof(),
>+ __glibcxx_requires_cond(_M_sbuf,
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
> if (_M_sbuf)
> {
>+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>+ int_type _tmp =
_tmp is not a reserved name, this needs to be __tmp.
I'm still reviewing the rest, to understand what observable behaviour
this changes, and how it differs from the patch François sent.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Make tests less istreambuf_iterator implementation dependent
@ 2017-09-27 20:16 François Dumont
2017-09-28 12:12 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-09-27 20:16 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 574 bytes --]
Hi
   I just committed attached patch as trivial.
   Those tests were highly istreambuf_iterator implementation, it is
the result of the call to money_get<>::get which is pointing immediately
beyond the last character recognized to quote Standard words.
2017-09-27 François Dumont <fdumont@gcc.gnu.org>
   * testsuite/22_locale/money_get/get/char/22131.cc: Make test less
   istreambuf_iterator implementation dependent.
   * testsuite/22_locale/money_get/get/wchar_t/22131.cc: Likewise.
François
[-- Attachment #2: tests.patch --]
[-- Type: text/x-patch, Size: 1218 bytes --]
diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/char/22131.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/char/22131.cc
index 6f363ef..5a05817 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/char/22131.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/char/22131.cc
@@ -67,7 +67,7 @@ void test01()
fmt2.imbue(loc);
InIt ibeg2(fmt2);
err2 = ios_base::goodbit;
- mg.get(ibeg2, iend2, intl, fmt2, err2, val2);
+ ibeg2 = mg.get(ibeg2, iend2, intl, fmt2, err2, val2);
VERIFY( err2 == ios_base::failbit );
VERIFY( *ibeg2 == '#' );
VERIFY( val2 == "" );
diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/22131.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/22131.cc
index 3830cc3..0626e3c 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/22131.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/22131.cc
@@ -67,7 +67,7 @@ void test01()
fmt2.imbue(loc);
InIt ibeg2(fmt2);
err2 = ios_base::goodbit;
- mg.get(ibeg2, iend2, intl, fmt2, err2, val2);
+ ibeg2 = mg.get(ibeg2, iend2, intl, fmt2, err2, val2);
VERIFY( err2 == ios_base::failbit );
VERIFY( *ibeg2 == L'#' );
VERIFY( val2 == L"" );
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf
2017-09-23 7:10 [PATCH] libstdc++: istreambuf_iterator keep attached streambuf Petr Ovtchenkov
2017-09-25 13:46 ` Jonathan Wakely
@ 2017-09-28 10:34 ` Jonathan Wakely
2017-09-28 12:06 ` Petr Ovtchenkov
1 sibling, 1 reply; 55+ messages in thread
From: Jonathan Wakely @ 2017-09-28 10:34 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
>istreambuf_iterator should not forget about attached
>streambuf when it reach EOF.
>
>Checks in debug mode has no infuence more on character
>extraction in istreambuf_iterator increment operators.
>In this aspect behaviour in debug and non-debug mode
>is similar now.
>
>Test for detached srteambuf in istreambuf_iterator:
>When istreambuf_iterator reach EOF of istream, it should not
>forget about attached streambuf.
From fact "EOF in stream reached" not follow that
>stream reach end of life and input operation impossible
>more.
>---
> libstdc++-v3/include/bits/streambuf_iterator.h | 41 +++++++--------
> .../24_iterators/istreambuf_iterator/3.cc | 61 ++++++++++++++++++++++
> 2 files changed, 80 insertions(+), 22 deletions(-)
> create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>
>diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
>index f0451b1..45c3d89 100644
>--- a/libstdc++-v3/include/bits/streambuf_iterator.h
>+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
>@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> istreambuf_iterator&
> operator++()
> {
>- __glibcxx_requires_cond(!_M_at_eof(),
>+ __glibcxx_requires_cond(_M_sbuf,
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
> if (_M_sbuf)
> {
>+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>+ int_type _tmp =
>+#endif
> _M_sbuf->sbumpc();
>+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>+ __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
>+ _M_message(__gnu_debug::__msg_inc_istreambuf)
>+ ._M_iterator(*this));
>+#endif
> _M_c = traits_type::eof();
> }
> return *this;
>@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> istreambuf_iterator
> operator++(int)
> {
>- __glibcxx_requires_cond(!_M_at_eof(),
>+ _M_get();
>+ __glibcxx_requires_cond(_M_sbuf
>+ && !traits_type::eq_int_type(_M_c,traits_type::eof()),
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
>
> istreambuf_iterator __old = *this;
> if (_M_sbuf)
> {
>- __old._M_c = _M_sbuf->sbumpc();
>+ _M_sbuf->sbumpc();
> _M_c = traits_type::eof();
> }
> return __old;
>@@ -177,18 +187,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _M_get() const
> {
> const int_type __eof = traits_type::eof();
>- int_type __ret = __eof;
>- if (_M_sbuf)
>- {
>- if (!traits_type::eq_int_type(_M_c, __eof))
>- __ret = _M_c;
>- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
>- __eof))
>- _M_c = __ret;
>- else
>- _M_sbuf = 0;
>- }
>- return __ret;
>+ if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
>+ _M_c = _M_sbuf->sgetc();
>+ return _M_c;
> }
>
> bool
>@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> typedef typename __is_iterator_type::streambuf_type streambuf_type;
> typedef typename traits_type::int_type int_type;
>
>- if (__first._M_sbuf && !__last._M_sbuf)
>+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> {
> streambuf_type* __sb = __first._M_sbuf;
> int_type __c = __sb->sgetc();
>@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> typedef typename __is_iterator_type::streambuf_type streambuf_type;
> typedef typename traits_type::int_type int_type;
>
>- if (__first._M_sbuf && !__last._M_sbuf)
>+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> {
> const int_type __ival = traits_type::to_int_type(__val);
> streambuf_type* __sb = __first._M_sbuf;
>@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> else
> __c = __sb->snextc();
> }
>-
>- if (!traits_type::eq_int_type(__c, traits_type::eof()))
>- __first._M_c = __c;
>- else
>- __first._M_sbuf = 0;
>+ __first._M_c = __c;
> }
> return __first;
> }
>diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>new file mode 100644
>index 0000000..803ede4
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>@@ -0,0 +1,61 @@
>+// { dg-options "-std=gnu++17" }
>+
>+// Copyright (C) 2017 Free Software Foundation, Inc.
>+//
>+// This file is part of the GNU ISO C++ Library. This library is free
>+// software; you can redistribute it and/or modify it under the
>+// terms of the GNU General Public License as published by the
>+// Free Software Foundation; either version 3, or (at your option)
>+// any later version.
>+
>+// This library is distributed in the hope that it will be useful,
>+// but WITHOUT ANY WARRANTY; without even the implied warranty of
>+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+// GNU General Public License for more details.
>+
>+// You should have received a copy of the GNU General Public License along
>+// with this library; see the file COPYING3. If not see
>+// <http://www.gnu.org/licenses/>.
>+
>+#include <algorithm>
>+#include <sstream>
>+#include <iterator>
>+#include <cstring>
>+#include <testsuite_hooks.h>
>+
>+void test03()
>+{
>+ using namespace std;
>+ bool test __attribute__((unused)) = true;
>+
>+ std::stringstream s;
>+ char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
>+ char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
>+ char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
>+ // 012345678901234567890123456789012345
>+ // 0 1 2 3
>+ s << b;
>+ VERIFY( !s.fail() );
>+
>+ istreambuf_iterator<char> i(s);
>+ copy_n(i, 36, r);
>+ ++i; // EOF reached
>+ 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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] streambuf_iterator: avoid debug-dependent behaviour
2017-09-21 5:46 ` François Dumont
@ 2017-09-28 10:50 ` Jonathan Wakely
2017-09-28 10:58 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: Jonathan Wakely @ 2017-09-28 10:50 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches, Petr Ovtchenkov
On 21/09/17 07:46 +0200, François Dumont wrote:
>Gentle reminder, ok to commit ?
No. Could you and Petr please come to an agreement about what is
actually wrong with the current implementation, and agree on a
solution?
Currently you're both just proposing patches that do different things,
without indicating why one patch is better than the other.
I understand that we want to remove the debugmode-dependent behaviour,
but I'd like to see any other changes justified by references to the
standard.
>diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
>index b81f4d4..e3d99f9 100644
>--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
>+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
>@@ -25,9 +25,7 @@
>
> void test02(void)
> {
>-
> typedef std::istreambuf_iterator<char> cistreambuf_iter;
>- typedef cistreambuf_iter::streambuf_type cstreambuf_type;
> const char slit01[] = "playa hermosa, liberia, guanacaste";
> std::string str01(slit01);
> std::istringstream istrs00(str01);
>@@ -35,10 +33,17 @@ void test02(void)
>
> // ctor sanity checks
> cistreambuf_iter istrb_it01(istrs00);
>- cistreambuf_iter istrb_it02;
>- std::string tmp(istrb_it01, istrb_it02);
>+ cistreambuf_iter istrb_eos;
>+ VERIFY( istrb_it01 != istrb_eos );
>+
>+ std::string tmp(istrb_it01, istrb_eos);
> VERIFY( tmp == str01 );
>
>+ VERIFY( istrb_it01 != istrb_eos );
Why should this condition be true? The std::string constructor
increments the iterator until it reaches the end-of-stream value.
This is true with our current implementation, but that seems like a
bug, not something we want to verify in the testsuite.
>+ cistreambuf_iter old = istrb_it01++;
>+ VERIFY( old == istrb_eos );
This behaviour makes no sense.
>+ VERIFY( istrb_it01 == istrb_eos );
>+
> cistreambuf_iter istrb_it03(0);
> cistreambuf_iter istrb_it04;
> VERIFY( istrb_it03 == istrb_it04 );
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] streambuf_iterator: avoid debug-dependent behaviour
2017-09-28 10:50 ` Jonathan Wakely
@ 2017-09-28 10:58 ` Jonathan Wakely
0 siblings, 0 replies; 55+ messages in thread
From: Jonathan Wakely @ 2017-09-28 10:58 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches, Petr Ovtchenkov
On 28/09/17 11:50 +0100, Jonathan Wakely wrote:
>On 21/09/17 07:46 +0200, François Dumont wrote:
>>Gentle reminder, ok to commit ?
>
>No. Could you and Petr please come to an agreement about what is
>actually wrong with the current implementation, and agree on a
>solution?
>
>Currently you're both just proposing patches that do different things,
>without indicating why one patch is better than the other.
>
>I understand that we want to remove the debugmode-dependent behaviour,
>but I'd like to see any other changes justified by references to the
>standard.
Here's a test we currently fail, but it looks like we should pass it:
http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp
Do the changes either of you is proposing change this result?
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf
2017-09-28 10:34 ` Jonathan Wakely
@ 2017-09-28 12:06 ` Petr Ovtchenkov
2017-09-28 12:38 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-09-28 12:06 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
On Thu, 28 Sep 2017 11:34:25 +0100
Jonathan Wakely <jwakely@redhat.com> wrote:
> On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
> >istreambuf_iterator should not forget about attached
> >streambuf when it reach EOF.
> >
> >Checks in debug mode has no infuence more on character
> >extraction in istreambuf_iterator increment operators.
> >In this aspect behaviour in debug and non-debug mode
> >is similar now.
> >
> >Test for detached srteambuf in istreambuf_iterator:
> >When istreambuf_iterator reach EOF of istream, it should not
> >forget about attached streambuf.
> From fact "EOF in stream reached" not follow that
> >stream reach end of life and input operation impossible
> >more.
> >---
> > libstdc++-v3/include/bits/streambuf_iterator.h | 41 +++++++--------
> > .../24_iterators/istreambuf_iterator/3.cc | 61 ++++++++++++++++++++++
> > 2 files changed, 80 insertions(+), 22 deletions(-)
> > create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >
> >diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
> >b/libstdc++-v3/include/bits/streambuf_iterator.h index f0451b1..45c3d89 100644
> >--- a/libstdc++-v3/include/bits/streambuf_iterator.h
> >+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> >@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > istreambuf_iterator&
> > operator++()
> > {
> >- __glibcxx_requires_cond(!_M_at_eof(),
> >+ __glibcxx_requires_cond(_M_sbuf,
> > _M_message(__gnu_debug::__msg_inc_istreambuf)
> > ._M_iterator(*this));
> > if (_M_sbuf)
> > {
> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
> >+ int_type _tmp =
> >+#endif
> > _M_sbuf->sbumpc();
> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
> >+ __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
> >+ _M_message(__gnu_debug::__msg_inc_istreambuf)
> >+ ._M_iterator(*this));
> >+#endif
> > _M_c = traits_type::eof();
> > }
> > return *this;
> >@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > istreambuf_iterator
> > operator++(int)
> > {
> >- __glibcxx_requires_cond(!_M_at_eof(),
> >+ _M_get();
> >+ __glibcxx_requires_cond(_M_sbuf
> >+ && !traits_type::eq_int_type(_M_c,traits_type::eof()),
> > _M_message(__gnu_debug::__msg_inc_istreambuf)
> > ._M_iterator(*this));
> >
> > istreambuf_iterator __old = *this;
> > if (_M_sbuf)
> > {
> >- __old._M_c = _M_sbuf->sbumpc();
> >+ _M_sbuf->sbumpc();
> > _M_c = traits_type::eof();
> > }
> > return __old;
> >@@ -177,18 +187,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > _M_get() const
> > {
> > const int_type __eof = traits_type::eof();
> >- int_type __ret = __eof;
> >- if (_M_sbuf)
> >- {
> >- if (!traits_type::eq_int_type(_M_c, __eof))
> >- __ret = _M_c;
> >- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
> >- __eof))
> >- _M_c = __ret;
> >- else
> >- _M_sbuf = 0;
> >- }
> >- return __ret;
> >+ if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
> >+ _M_c = _M_sbuf->sgetc();
> >+ return _M_c;
> > }
> >
> > bool
> >@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > typedef typename __is_iterator_type::streambuf_type streambuf_type;
> > typedef typename traits_type::int_type int_type;
> >
> >- if (__first._M_sbuf && !__last._M_sbuf)
> >+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> > {
> > streambuf_type* __sb = __first._M_sbuf;
> > int_type __c = __sb->sgetc();
> >@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > typedef typename __is_iterator_type::streambuf_type streambuf_type;
> > typedef typename traits_type::int_type int_type;
> >
> >- if (__first._M_sbuf && !__last._M_sbuf)
> >+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> > {
> > const int_type __ival = traits_type::to_int_type(__val);
> > streambuf_type* __sb = __first._M_sbuf;
> >@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > else
> > __c = __sb->snextc();
> > }
> >-
> >- if (!traits_type::eq_int_type(__c, traits_type::eof()))
> >- __first._M_c = __c;
> >- else
> >- __first._M_sbuf = 0;
> >+ __first._M_c = __c;
> > }
> > return __first;
> > }
> >diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc new file mode 100644
> >index 0000000..803ede4
> >--- /dev/null
> >+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >@@ -0,0 +1,61 @@
> >+// { dg-options "-std=gnu++17" }
> >+
> >+// Copyright (C) 2017 Free Software Foundation, Inc.
> >+//
> >+// This file is part of the GNU ISO C++ Library. This library is free
> >+// software; you can redistribute it and/or modify it under the
> >+// terms of the GNU General Public License as published by the
> >+// Free Software Foundation; either version 3, or (at your option)
> >+// any later version.
> >+
> >+// This library is distributed in the hope that it will be useful,
> >+// but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >+// GNU General Public License for more details.
> >+
> >+// You should have received a copy of the GNU General Public License along
> >+// with this library; see the file COPYING3. If not see
> >+// <http://www.gnu.org/licenses/>.
> >+
> >+#include <algorithm>
> >+#include <sstream>
> >+#include <iterator>
> >+#include <cstring>
> >+#include <testsuite_hooks.h>
> >+
> >+void test03()
> >+{
> >+ using namespace std;
> >+ bool test __attribute__((unused)) = true;
> >+
> >+ std::stringstream s;
> >+ char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
> >+ char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
> >+ char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
> >+ // 012345678901234567890123456789012345
> >+ // 0 1 2 3
> >+ s << b;
> >+ VERIFY( !s.fail() );
> >+
> >+ istreambuf_iterator<char> i(s);
> >+ copy_n(i, 36, r);
> >+ ++i; // EOF reached
> >+ 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.
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.
debugmode-dependent behaviour is also issue in this patch;
I don't suggest it as a separate patch because solutions are intersect.
WBR,
--
- ptr
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make tests less istreambuf_iterator implementation dependent
2017-09-27 20:16 Make tests less istreambuf_iterator implementation dependent François Dumont
@ 2017-09-28 12:12 ` Jonathan Wakely
2017-09-28 19:59 ` François Dumont
0 siblings, 1 reply; 55+ messages in thread
From: Jonathan Wakely @ 2017-09-28 12:12 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On 27/09/17 22:16 +0200, François Dumont wrote:
>Hi
>
>Â Â Â I just committed attached patch as trivial.
>
>Â Â Â Those tests were highly istreambuf_iterator implementation, it is
>the result of the call to money_get<>::get which is pointing
>immediately beyond the last character recognized to quote Standard
>words.
But according to the standard's specification for istreambuf_iterator
it makes no difference, because both iterators point to the same
streambuf and share the state.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf
2017-09-28 12:06 ` Petr Ovtchenkov
@ 2017-09-28 12:38 ` Jonathan Wakely
2017-10-03 20:39 ` Petr Ovtchenkov
0 siblings, 1 reply; 55+ messages in thread
From: Jonathan Wakely @ 2017-09-28 12:38 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
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:
>
>> On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
>> >istreambuf_iterator should not forget about attached
>> >streambuf when it reach EOF.
>> >
>> >Checks in debug mode has no infuence more on character
>> >extraction in istreambuf_iterator increment operators.
>> >In this aspect behaviour in debug and non-debug mode
>> >is similar now.
>> >
>> >Test for detached srteambuf in istreambuf_iterator:
>> >When istreambuf_iterator reach EOF of istream, it should not
>> >forget about attached streambuf.
>> From fact "EOF in stream reached" not follow that
>> >stream reach end of life and input operation impossible
>> >more.
>> >---
>> > libstdc++-v3/include/bits/streambuf_iterator.h | 41 +++++++--------
>> > .../24_iterators/istreambuf_iterator/3.cc | 61 ++++++++++++++++++++++
>> > 2 files changed, 80 insertions(+), 22 deletions(-)
>> > create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>> >
>> >diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
>> >b/libstdc++-v3/include/bits/streambuf_iterator.h index f0451b1..45c3d89 100644
>> >--- a/libstdc++-v3/include/bits/streambuf_iterator.h
>> >+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
>> >@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> > istreambuf_iterator&
>> > operator++()
>> > {
>> >- __glibcxx_requires_cond(!_M_at_eof(),
>> >+ __glibcxx_requires_cond(_M_sbuf,
>> > _M_message(__gnu_debug::__msg_inc_istreambuf)
>> > ._M_iterator(*this));
>> > if (_M_sbuf)
>> > {
>> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>> >+ int_type _tmp =
>> >+#endif
>> > _M_sbuf->sbumpc();
>> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>> >+ __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
>> >+ _M_message(__gnu_debug::__msg_inc_istreambuf)
>> >+ ._M_iterator(*this));
>> >+#endif
>> > _M_c = traits_type::eof();
>> > }
>> > return *this;
>> >@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> > istreambuf_iterator
>> > operator++(int)
>> > {
>> >- __glibcxx_requires_cond(!_M_at_eof(),
>> >+ _M_get();
>> >+ __glibcxx_requires_cond(_M_sbuf
>> >+ && !traits_type::eq_int_type(_M_c,traits_type::eof()),
>> > _M_message(__gnu_debug::__msg_inc_istreambuf)
>> > ._M_iterator(*this));
>> >
>> > istreambuf_iterator __old = *this;
>> > if (_M_sbuf)
>> > {
>> >- __old._M_c = _M_sbuf->sbumpc();
>> >+ _M_sbuf->sbumpc();
>> > _M_c = traits_type::eof();
>> > }
>> > return __old;
>> >@@ -177,18 +187,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> > _M_get() const
>> > {
>> > const int_type __eof = traits_type::eof();
>> >- int_type __ret = __eof;
>> >- if (_M_sbuf)
>> >- {
>> >- if (!traits_type::eq_int_type(_M_c, __eof))
>> >- __ret = _M_c;
>> >- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
>> >- __eof))
>> >- _M_c = __ret;
>> >- else
>> >- _M_sbuf = 0;
>> >- }
>> >- return __ret;
>> >+ if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
>> >+ _M_c = _M_sbuf->sgetc();
>> >+ return _M_c;
>> > }
>> >
>> > bool
>> >@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> > typedef typename __is_iterator_type::streambuf_type streambuf_type;
>> > typedef typename traits_type::int_type int_type;
>> >
>> >- if (__first._M_sbuf && !__last._M_sbuf)
>> >+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
>> > {
>> > streambuf_type* __sb = __first._M_sbuf;
>> > int_type __c = __sb->sgetc();
>> >@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> > typedef typename __is_iterator_type::streambuf_type streambuf_type;
>> > typedef typename traits_type::int_type int_type;
>> >
>> >- if (__first._M_sbuf && !__last._M_sbuf)
>> >+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
>> > {
>> > const int_type __ival = traits_type::to_int_type(__val);
>> > streambuf_type* __sb = __first._M_sbuf;
>> >@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> > else
>> > __c = __sb->snextc();
>> > }
>> >-
>> >- if (!traits_type::eq_int_type(__c, traits_type::eof()))
>> >- __first._M_c = __c;
>> >- else
>> >- __first._M_sbuf = 0;
>> >+ __first._M_c = __c;
>> > }
>> > return __first;
>> > }
>> >diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>> >b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc new file mode 100644
>> >index 0000000..803ede4
>> >--- /dev/null
>> >+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>> >@@ -0,0 +1,61 @@
>> >+// { dg-options "-std=gnu++17" }
>> >+
>> >+// Copyright (C) 2017 Free Software Foundation, Inc.
>> >+//
>> >+// This file is part of the GNU ISO C++ Library. This library is free
>> >+// software; you can redistribute it and/or modify it under the
>> >+// terms of the GNU General Public License as published by the
>> >+// Free Software Foundation; either version 3, or (at your option)
>> >+// any later version.
>> >+
>> >+// This library is distributed in the hope that it will be useful,
>> >+// but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> >+// GNU General Public License for more details.
>> >+
>> >+// You should have received a copy of the GNU General Public License along
>> >+// with this library; see the file COPYING3. If not see
>> >+// <http://www.gnu.org/licenses/>.
>> >+
>> >+#include <algorithm>
>> >+#include <sstream>
>> >+#include <iterator>
>> >+#include <cstring>
>> >+#include <testsuite_hooks.h>
>> >+
>> >+void test03()
>> >+{
>> >+ using namespace std;
>> >+ bool test __attribute__((unused)) = true;
This variable should be removed, we don't need these variables any
more.
>> >+ std::stringstream s;
>> >+ char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
>> >+ char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
>> >+ char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
>> >+ // 012345678901234567890123456789012345
>> >+ // 0 1 2 3
>> >+ s << b;
>> >+ VERIFY( !s.fail() );
>> >+
>> >+ istreambuf_iterator<char> i(s);
>> >+ copy_n(i, 36, r);
>> >+ ++i; // EOF reached
>> >+ 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.
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.
>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.
>debugmode-dependent behaviour is also issue in this patch;
>I don't suggest it as a separate patch because solutions are intersect.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make tests less istreambuf_iterator implementation dependent
2017-09-28 12:12 ` Jonathan Wakely
@ 2017-09-28 19:59 ` François Dumont
2017-09-28 21:56 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-09-28 19:59 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
On 28/09/2017 14:12, Jonathan Wakely wrote:
> On 27/09/17 22:16 +0200, François Dumont wrote:
>> Hi
>>
>> Â Â Â I just committed attached patch as trivial.
>>
>> Â Â Â Those tests were highly istreambuf_iterator implementation, it is
>> the result of the call to money_get<>::get which is pointing
>> immediately beyond the last character recognized to quote Standard
>> words.
>
> But according to the standard's specification for istreambuf_iterator
> it makes no difference, because both iterators point to the same
> streambuf and share the state.
>
>
Simply adding a:
VERIFY( *ibeg2 == '0' );
before calling money_get<>::get method would have broken the test.
The current istreambuf_iterator implementation capture the current
streambuf state each time it is tested for eof or evaluated. This is why
I considered those tests as fragile.
François
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make tests less istreambuf_iterator implementation dependent
2017-09-28 19:59 ` François Dumont
@ 2017-09-28 21:56 ` Jonathan Wakely
2017-10-02 5:43 ` François Dumont
0 siblings, 1 reply; 55+ messages in thread
From: Jonathan Wakely @ 2017-09-28 21:56 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On 28/09/17 21:59 +0200, François Dumont wrote:
>On 28/09/2017 14:12, Jonathan Wakely wrote:
>>On 27/09/17 22:16 +0200, François Dumont wrote:
>>>Hi
>>>
>>>Â Â Â I just committed attached patch as trivial.
>>>
>>>Â Â Â Those tests were highly istreambuf_iterator implementation, it
>>>is the result of the call to money_get<>::get which is pointing
>>>immediately beyond the last character recognized to quote Standard
>>>words.
>>
>>But according to the standard's specification for istreambuf_iterator
>>it makes no difference, because both iterators point to the same
>>streambuf and share the state.
>>
>>
>Simply adding a:
>
>VERIFY( *ibeg2 == '0' );
>
>before calling money_get<>::get method would have broken the test.
>
>The current istreambuf_iterator implementation capture the current
>streambuf state each time it is tested for eof or evaluated. This is
>why I considered those tests as fragile.
Yes, and I think that's non-conforming.
As I said in the other thread, I'd really like to see references to
the standard used to justify any changes to our istreambuf_iterator.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make tests less istreambuf_iterator implementation dependent
2017-09-28 21:56 ` Jonathan Wakely
@ 2017-10-02 5:43 ` François Dumont
2017-10-03 14:20 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-10-02 5:43 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]
On 28/09/2017 23:56, Jonathan Wakely wrote:
> On 28/09/17 21:59 +0200, François Dumont wrote:
>>
>> The current istreambuf_iterator implementation capture the current
>> streambuf state each time it is tested for eof or evaluated. This is
>> why I considered those tests as fragile.
>
> Yes, and I think that's non-conforming.
>
Good, then we have something to fix.
> As I said in the other thread, I'd really like to see references to
> the standard used to justify any changes to our istreambuf_iterator
I think _M_c has been introduced to cover this paragraph of the Standard:
24.6.3.1
"1. Class istreambuf_iterator<charT,traits>::proxy is for exposition
only. An implementation is permit-
ted to provide equivalent functionality without providing a class with
this name. Class istreambuf_-
iterator<charT, traits>::proxy provides a temporary placeholder as the
return value of the post-
increment operator (operator++). It keeps the character pointed to by
the previous value of the iterator for
some possible future access to get the character."
This is why it is being set in operator++(int):
   istreambuf_iterator __old = *this;
   __old._M_c = _M_sbuf->sbumpc();
It is also the reason why libstdc++ fails:
http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp
It looks like this test is simply not Standard conformant.
I guess at some point _M_c started to be used also to cache the
streambuf::sgetc resulting in current situation.
In attached patch I limit usage of _M_c to cover 24.6.3.1.1 point and so
made additional changes to 2.cc test case to demonstrate it.
François
[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5232 bytes --]
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..94d8c7f 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,7 +95,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// NB: This implementation assumes the "end of stream" value
// is EOF, or -1.
mutable streambuf_type* _M_sbuf;
- mutable int_type _M_c;
+ int_type _M_c;
public:
/// Construct end of input stream iterator.
@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
: _M_sbuf(0), _M_c(traits_type::eof()) { }
#if __cplusplus >= 201103L
- istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+ istreambuf_iterator(const istreambuf_iterator&) = default;
~istreambuf_iterator() = default;
#endif
@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
char_type
operator*() const
{
+ int_type __c = _M_get();
+
#ifdef _GLIBCXX_DEBUG_PEDANTIC
// Dereferencing a past-the-end istreambuf_iterator is a
// libstdc++ extension
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(!_S_at_eof(__c),
_M_message(__gnu_debug::__msg_deref_istreambuf)
._M_iterator(*this));
#endif
- return traits_type::to_char_type(_M_get());
+ return traits_type::to_char_type(__c);
}
/// Advance the iterator. Calls streambuf.sbumpc().
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf &&
+ (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
- if (_M_sbuf)
- {
+
_M_sbuf->sbumpc();
_M_c = traits_type::eof();
- }
return *this;
}
@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf &&
+ (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
istreambuf_iterator __old = *this;
- if (_M_sbuf)
- {
__old._M_c = _M_sbuf->sbumpc();
_M_c = traits_type::eof();
- }
return __old;
}
@@ -176,26 +175,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
int_type
_M_get() const
{
- const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
- if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
+ int_type __ret = _M_c;
+ if (_M_sbuf && _S_at_eof(__ret) && _S_at_eof(__ret = _M_sbuf->sgetc()))
_M_sbuf = 0;
- }
return __ret;
}
bool
_M_at_eof() const
+ { return _S_at_eof(_M_get()); }
+
+ static bool
+ _S_at_eof(int_type __c)
{
const int_type __eof = traits_type::eof();
- return traits_type::eq_int_type(_M_get(), __eof);
+ return traits_type::eq_int_type(__c, __eof);
}
};
@@ -373,13 +367,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::traits_type traits_type;
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
+ const int_type __eof = traits_type::eof();
if (__first._M_sbuf && !__last._M_sbuf)
{
const int_type __ival = traits_type::to_int_type(__val);
streambuf_type* __sb = __first._M_sbuf;
int_type __c = __sb->sgetc();
- while (!traits_type::eq_int_type(__c, traits_type::eof())
+ while (!traits_type::eq_int_type(__c, __eof)
&& !traits_type::eq_int_type(__c, __ival))
{
streamsize __n = __sb->egptr() - __sb->gptr();
@@ -396,11 +391,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__c = __sb->snextc();
}
- if (!traits_type::eq_int_type(__c, traits_type::eof()))
- __first._M_c = __c;
- else
- __first._M_sbuf = 0;
+ __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 b81f4d4..572ea9f 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
void test02(void)
{
-
typedef std::istreambuf_iterator<char> cistreambuf_iter;
- typedef cistreambuf_iter::streambuf_type cstreambuf_type;
const char slit01[] = "playa hermosa, liberia, guanacaste";
std::string str01(slit01);
std::istringstream istrs00(str01);
@@ -35,10 +33,14 @@ void test02(void)
// ctor sanity checks
cistreambuf_iter istrb_it01(istrs00);
- cistreambuf_iter istrb_it02;
- std::string tmp(istrb_it01, istrb_it02);
+ cistreambuf_iter istrb_eos;
+ VERIFY( istrb_it01 != istrb_eos );
+
+ std::string tmp(istrb_it01, istrb_eos);
VERIFY( tmp == str01 );
+ VERIFY( istrb_it01 == istrb_eos );
+
cistreambuf_iter istrb_it03(0);
cistreambuf_iter istrb_it04;
VERIFY( istrb_it03 == istrb_it04 );
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make tests less istreambuf_iterator implementation dependent
2017-10-02 5:43 ` François Dumont
@ 2017-10-03 14:20 ` Jonathan Wakely
2017-10-04 16:21 ` François Dumont
0 siblings, 1 reply; 55+ messages in thread
From: Jonathan Wakely @ 2017-10-03 14:20 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On 02/10/17 07:43 +0200, François Dumont wrote:
>On 28/09/2017 23:56, Jonathan Wakely wrote:
>>On 28/09/17 21:59 +0200, François Dumont wrote:
>>>
>>>The current istreambuf_iterator implementation capture the current
>>>streambuf state each time it is tested for eof or evaluated. This
>>>is why I considered those tests as fragile.
>>
>>Yes, and I think that's non-conforming.
>>
>
>Good, then we have something to fix.
>
>>As I said in the other thread, I'd really like to see references to
>>the standard used to justify any changes to our istreambuf_iterator
>
>I think _M_c has been introduced to cover this paragraph of the Standard:
>
>24.6.3.1
>
>"1. Class istreambuf_iterator<charT,traits>::proxy is for exposition
>only. An implementation is permit-
>ted to provide equivalent functionality without providing a class with
>this name. Class istreambuf_-
>iterator<charT, traits>::proxy provides a temporary placeholder as the
>return value of the post-
>increment operator (operator++). It keeps the character pointed to by
>the previous value of the iterator for
>some possible future access to get the character."
>
>This is why it is being set in operator++(int):
>
>Â Â Â istreambuf_iterator __old = *this;
>Â Â Â __old._M_c = _M_sbuf->sbumpc();
>
>It is also the reason why libstdc++ fails:
>http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp
>
>
>It looks like this test is simply not Standard conformant.
I think you're right, because it relies on a property that may not be
true:
any copies of the previous
value of r are no longer
required either to be
dereferenceable or to be in
the domain of ==.
>I guess at some point _M_c started to be used also to cache the
>streambuf::sgetc resulting in current situation.
And arguably that is not "equivalent functionality" to returning a
proxy object. Although the standard seems a bit underspecified.
>In attached patch I limit usage of _M_c to cover 24.6.3.1.1 point and
>so made additional changes to 2.cc test case to demonstrate it.
Doesn't the modified test PASS anyway, even without changing how _M_c
is used?
Generally this looks good (I'm not sure if it addresses Petr's
concerns, but I don't think his "stop and go" usage is valid anyway -
it's certainly not portable or guaranteed by the standard).
>@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> : _M_sbuf(0), _M_c(traits_type::eof()) { }
>
> #if __cplusplus >= 201103L
>- istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
>+ istreambuf_iterator(const istreambuf_iterator&) = default;
Please don't remove this.
It's present in the standard, and it ensures we don't accidentally add
a member that makes the default noexcept(false).
>
> ~istreambuf_iterator() = default;
> #endif
>@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> char_type
> operator*() const
> {
>+ int_type __c = _M_get();
>+
> #ifdef _GLIBCXX_DEBUG_PEDANTIC
> // Dereferencing a past-the-end istreambuf_iterator is a
> // libstdc++ extension
>- __glibcxx_requires_cond(!_M_at_eof(),
>+ __glibcxx_requires_cond(!_S_at_eof(__c),
> _M_message(__gnu_debug::__msg_deref_istreambuf)
> ._M_iterator(*this));
> #endif
>- return traits_type::to_char_type(_M_get());
>+ return traits_type::to_char_type(__c);
> }
>
> /// Advance the iterator. Calls streambuf.sbumpc().
> istreambuf_iterator&
> operator++()
> {
>- __glibcxx_requires_cond(!_M_at_eof(),
>+ __glibcxx_requires_cond(_M_sbuf &&
>+ (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
>- if (_M_sbuf)
>- {
This check should be unnecessary, because it's undefined to increment
and end-of-stream iterator, but I wonder if anyone is relying on it
being currently a no-op, rather than crashing.
Let's remove the check, and if it causes too many problems we can
restore it as:
if (__builtin_expect(_M_sbuf != 0, 1))
>+
> _M_sbuf->sbumpc();
> _M_c = traits_type::eof();
>- }
> return *this;
> }
>
>@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> istreambuf_iterator
> operator++(int)
> {
>- __glibcxx_requires_cond(!_M_at_eof(),
>+ __glibcxx_requires_cond(_M_sbuf &&
>+ (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
>
> istreambuf_iterator __old = *this;
>- if (_M_sbuf)
>- {
> __old._M_c = _M_sbuf->sbumpc();
> _M_c = traits_type::eof();
>- }
> return __old;
> }
>
>@@ -176,26 +175,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> int_type
> _M_get() const
> {
>- const int_type __eof = traits_type::eof();
>- int_type __ret = __eof;
>- if (_M_sbuf)
>- {
>- if (!traits_type::eq_int_type(_M_c, __eof))
>- __ret = _M_c;
>- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
>- __eof))
>- _M_c = __ret;
Removing this assignment means that we will call _M_sbuf->sgetc() for
every dereference operation, instead of caching the current value in
_M_c. I think that's probably OK, because it's unlikely that
performance critical code is dereferencing the same iterator
repeatedly without incrementing it.
And getting rid of the mutable member is a Good Thing.
An alternative would be to cache it in operator*() instead of in
_M_get(), which would still give the desired change of not updating it
when _M_get() is used elsewhere. But would still involve setting a
mutable member in a const function.
>- else
>+ int_type __ret = _M_c;
>+ if (_M_sbuf && _S_at_eof(__ret) && _S_at_eof(__ret = _M_sbuf->sgetc()))
> _M_sbuf = 0;
It's unfortunate that we still have this assignment to a mutable
member in a const function, but I think it's necessary to avoid an
end-of-stream iterator becoming valid again.
>- }
> return __ret;
> }
>
> bool
> _M_at_eof() const
>+ { return _S_at_eof(_M_get()); }
>+
>+ static bool
>+ _S_at_eof(int_type __c)
This should be called _S_is_eof, since it tests if the argument **is**
the EOF character. It doesn't test if the stream is **at** EOF, like
_M_at_eof does.
So with a suitable ChangeLog, the "noexcept" kept on the constructor,
and renaming _S_at_eof to _S_is_eof, this is OK for trunk.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf
2017-09-28 12:38 ` Jonathan Wakely
@ 2017-10-03 20:39 ` Petr Ovtchenkov
2017-10-04 5:04 ` [PATCH v2] " Petr Ovtchenkov
2017-10-06 16:01 ` [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf) François Dumont
0 siblings, 2 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-10-03 20:39 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
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:
> >
> >> On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
> >> >istreambuf_iterator should not forget about attached
> >> >streambuf when it reach EOF.
> >> >
> >> >Checks in debug mode has no infuence more on character
> >> >extraction in istreambuf_iterator increment operators.
> >> >In this aspect behaviour in debug and non-debug mode
> >> >is similar now.
> >> >
> >> >Test for detached srteambuf in istreambuf_iterator:
> >> >When istreambuf_iterator reach EOF of istream, it should not
> >> >forget about attached streambuf.
> >> From fact "EOF in stream reached" not follow that
> >> >stream reach end of life and input operation impossible
> >> >more.
> >> >---
> >> > libstdc++-v3/include/bits/streambuf_iterator.h | 41 +++++++--------
> >> > .../24_iterators/istreambuf_iterator/3.cc | 61 ++++++++++++++++++++++
> >> > 2 files changed, 80 insertions(+), 22 deletions(-)
> >> > create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >> >
> >> >diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
> >> >b/libstdc++-v3/include/bits/streambuf_iterator.h index f0451b1..45c3d89 100644
> >> >--- a/libstdc++-v3/include/bits/streambuf_iterator.h
> >> >+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> >> >@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> > istreambuf_iterator&
> >> > operator++()
> >> > {
> >> >- __glibcxx_requires_cond(!_M_at_eof(),
> >> >+ __glibcxx_requires_cond(_M_sbuf,
> >> > _M_message(__gnu_debug::__msg_inc_istreambuf)
> >> > ._M_iterator(*this));
> >> > if (_M_sbuf)
> >> > {
> >> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
> >> >+ int_type _tmp =
> >> >+#endif
> >> > _M_sbuf->sbumpc();
> >> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
> >> >+ __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
> >> >+ _M_message(__gnu_debug::__msg_inc_istreambuf)
> >> >+ ._M_iterator(*this));
> >> >+#endif
> >> > _M_c = traits_type::eof();
> >> > }
> >> > return *this;
> >> >@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> > istreambuf_iterator
> >> > operator++(int)
> >> > {
> >> >- __glibcxx_requires_cond(!_M_at_eof(),
> >> >+ _M_get();
> >> >+ __glibcxx_requires_cond(_M_sbuf
> >> >+ && !traits_type::eq_int_type(_M_c,traits_type::eof()),
> >> > _M_message(__gnu_debug::__msg_inc_istreambuf)
> >> > ._M_iterator(*this));
> >> >
> >> > istreambuf_iterator __old = *this;
> >> > if (_M_sbuf)
> >> > {
> >> >- __old._M_c = _M_sbuf->sbumpc();
> >> >+ _M_sbuf->sbumpc();
> >> > _M_c = traits_type::eof();
> >> > }
> >> > return __old;
> >> >@@ -177,18 +187,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> > _M_get() const
> >> > {
> >> > const int_type __eof = traits_type::eof();
> >> >- int_type __ret = __eof;
> >> >- if (_M_sbuf)
> >> >- {
> >> >- if (!traits_type::eq_int_type(_M_c, __eof))
> >> >- __ret = _M_c;
> >> >- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
> >> >- __eof))
> >> >- _M_c = __ret;
> >> >- else
> >> >- _M_sbuf = 0;
> >> >- }
> >> >- return __ret;
> >> >+ if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
> >> >+ _M_c = _M_sbuf->sgetc();
> >> >+ return _M_c;
> >> > }
> >> >
> >> > bool
> >> >@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> > typedef typename __is_iterator_type::streambuf_type streambuf_type;
> >> > typedef typename traits_type::int_type int_type;
> >> >
> >> >- if (__first._M_sbuf && !__last._M_sbuf)
> >> >+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> >> > {
> >> > streambuf_type* __sb = __first._M_sbuf;
> >> > int_type __c = __sb->sgetc();
> >> >@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> > typedef typename __is_iterator_type::streambuf_type streambuf_type;
> >> > typedef typename traits_type::int_type int_type;
> >> >
> >> >- if (__first._M_sbuf && !__last._M_sbuf)
> >> >+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> >> > {
> >> > const int_type __ival = traits_type::to_int_type(__val);
> >> > streambuf_type* __sb = __first._M_sbuf;
> >> >@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> > else
> >> > __c = __sb->snextc();
> >> > }
> >> >-
> >> >- if (!traits_type::eq_int_type(__c, traits_type::eof()))
> >> >- __first._M_c = __c;
> >> >- else
> >> >- __first._M_sbuf = 0;
> >> >+ __first._M_c = __c;
> >> > }
> >> > return __first;
> >> > }
> >> >diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >> >b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc new file mode 100644
> >> >index 0000000..803ede4
> >> >--- /dev/null
> >> >+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >> >@@ -0,0 +1,61 @@
> >> >+// { dg-options "-std=gnu++17" }
> >> >+
> >> >+// Copyright (C) 2017 Free Software Foundation, Inc.
> >> >+//
> >> >+// This file is part of the GNU ISO C++ Library. This library is free
> >> >+// software; you can redistribute it and/or modify it under the
> >> >+// terms of the GNU General Public License as published by the
> >> >+// Free Software Foundation; either version 3, or (at your option)
> >> >+// any later version.
> >> >+
> >> >+// This library is distributed in the hope that it will be useful,
> >> >+// but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> >+// GNU General Public License for more details.
> >> >+
> >> >+// You should have received a copy of the GNU General Public License along
> >> >+// with this library; see the file COPYING3. If not see
> >> >+// <http://www.gnu.org/licenses/>.
> >> >+
> >> >+#include <algorithm>
> >> >+#include <sstream>
> >> >+#include <iterator>
> >> >+#include <cstring>
> >> >+#include <testsuite_hooks.h>
> >> >+
> >> >+void test03()
> >> >+{
> >> >+ using namespace std;
> >> >+ bool test __attribute__((unused)) = true;
>
> This variable should be removed, we don't need these variables any
> more.
Not a problem, if we will have consensus in principle.
>
> >> >+ std::stringstream s;
> >> >+ char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
> >> >+ char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
> >> >+ char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
> >> >+ // 012345678901234567890123456789012345
> >> >+ // 0 1 2 3
> >> >+ s << b;
> >> >+ VERIFY( !s.fail() );
> >> >+
> >> >+ istreambuf_iterator<char> i(s);
> >> >+ copy_n(i, 36, r);
> >> >+ ++i; // EOF reached
> >> >+ 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.
>
> >debugmode-dependent behaviour is also issue in this patch;
> >I don't suggest it as a separate patch because solutions are intersect.
We have three issues with istreambuf_iterator:
- debug-dependent behaviour
- EOL of istreambuf_iterator when it reach EOF (but this not mean
EOL of associated streambuf)
- postfix increment operator return istreambuf_iterator, but here
expected restricted type, that accept only dereference, if it possible.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2] libstdc++: istreambuf_iterator keep attached streambuf
2017-10-03 20:39 ` Petr Ovtchenkov
@ 2017-10-04 5:04 ` Petr Ovtchenkov
2017-10-06 16:01 ` [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf) François Dumont
1 sibling, 0 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-10-04 5:04 UTC (permalink / raw)
To: libstdc++, Jonathan Wakely; +Cc: gcc-patches
istreambuf_iterator should not forget about attached
streambuf when it reach EOF.
Checks in debug mode has no infuence more on character
extraction in istreambuf_iterator increment operators.
In this aspect behaviour in debug and non-debug mode
is similar now.
Test for detached srteambuf in istreambuf_iterator:
When istreambuf_iterator reach EOF of istream, it should not
forget about attached streambuf.
From fact "EOF in stream reached" not follow that
stream reach end of life and input operation impossible
more.
postfix increment (r++) return isb_iterator_proxy, due to
copies of the previous value of r are no longer
required either to be dereferenceable or to be in
the domain of ==.
i.e. type that usable only for dereference and extraction
"previous" character.
---
libstdc++-v3/include/bits/streambuf_iterator.h | 60 ++++++++++++--------
.../24_iterators/istreambuf_iterator/3.cc | 64 ++++++++++++++++++++++
2 files changed, 100 insertions(+), 24 deletions(-)
create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..b71bdd2 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -97,6 +97,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
mutable streambuf_type* _M_sbuf;
mutable int_type _M_c;
+ class isb_iterator_proxy
+ {
+ friend class istreambuf_iterator;
+ private:
+ isb_iterator_proxy(int_type c) :
+ _M_c(c)
+ { }
+ int_type _M_c;
+
+ public:
+ char_type
+ operator*() const
+ { return traits_type::to_char_type(_M_c); }
+ };
+
public:
/// Construct end of input stream iterator.
_GLIBCXX_CONSTEXPR istreambuf_iterator() _GLIBCXX_USE_NOEXCEPT
@@ -136,29 +151,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf,
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
if (_M_sbuf)
{
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+ int_type __tmp =
+#endif
_M_sbuf->sbumpc();
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+ __glibcxx_requires_cond(!traits_type::eq_int_type(__tmp,traits_type::eof()),
+ _M_message(__gnu_debug::__msg_inc_istreambuf)
+ ._M_iterator(*this));
+#endif
_M_c = traits_type::eof();
}
return *this;
}
/// Advance the iterator. Calls streambuf.sbumpc().
- istreambuf_iterator
+ isb_iterator_proxy
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ _M_get();
+ __glibcxx_requires_cond(_M_sbuf
+ && !traits_type::eq_int_type(_M_c,traits_type::eof()),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
- istreambuf_iterator __old = *this;
+ isb_iterator_proxy __old(_M_c);
if (_M_sbuf)
{
- __old._M_c = _M_sbuf->sbumpc();
+ _M_sbuf->sbumpc();
_M_c = traits_type::eof();
}
return __old;
@@ -177,18 +202,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_get() const
{
const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
- if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
- _M_sbuf = 0;
- }
- return __ret;
+ if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
+ _M_c = _M_sbuf->sgetc();
+ return _M_c;
}
bool
@@ -339,7 +355,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
- if (__first._M_sbuf && !__last._M_sbuf)
+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
{
streambuf_type* __sb = __first._M_sbuf;
int_type __c = __sb->sgetc();
@@ -374,7 +390,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
- if (__first._M_sbuf && !__last._M_sbuf)
+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
{
const int_type __ival = traits_type::to_int_type(__val);
streambuf_type* __sb = __first._M_sbuf;
@@ -395,11 +411,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
else
__c = __sb->snextc();
}
-
- if (!traits_type::eq_int_type(__c, traits_type::eof()))
- __first._M_c = __c;
- else
- __first._M_sbuf = 0;
+ __first._M_c = __c;
}
return __first;
}
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
new file mode 100644
index 0000000..5792e0d
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
@@ -0,0 +1,64 @@
+// { dg-options "-std=gnu++17" }
+
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+#include <cstring>
+#include <testsuite_hooks.h>
+
+void test03()
+{
+ using namespace std;
+
+ std::stringstream s;
+ char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
+ char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
+ char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
+ // 012345678901234567890123456789012345
+ // 0 1 2 3
+ s << b;
+ VERIFY( !s.fail() );
+
+ istreambuf_iterator<char> i(s);
+ copy_n(i, 36, r);
+ ++i; // EOF reached
+ VERIFY(i == std::istreambuf_iterator<char>());
+
+ VERIFY(memcmp(b, r, 36) == 0);
+
+ s << q;
+ VERIFY(!s.fail());
+
+ copy_n(i, 36, r);
+ ++i; // EOF reached
+ VERIFY(i == std::istreambuf_iterator<char>());
+
+ VERIFY(memcmp(q, r, 36) == 0);
+
+ s << 'Q';
+
+ VERIFY(*i++ == 'Q');
+}
+
+int main()
+{
+ test03();
+ return 0;
+}
--
2.10.1
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make tests less istreambuf_iterator implementation dependent
2017-10-03 14:20 ` Jonathan Wakely
@ 2017-10-04 16:21 ` François Dumont
2017-10-04 23:23 ` Jonathan Wakely
2017-11-15 20:54 ` [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" Petr Ovtchenkov
0 siblings, 2 replies; 55+ messages in thread
From: François Dumont @ 2017-10-04 16:21 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 7101 bytes --]
On 03/10/2017 16:20, Jonathan Wakely wrote:
> On 02/10/17 07:43 +0200, François Dumont wrote:
>> On 28/09/2017 23:56, Jonathan Wakely wrote:
>>> On 28/09/17 21:59 +0200, François Dumont wrote:
>>>>
>>>> The current istreambuf_iterator implementation capture the current
>>>> streambuf state each time it is tested for eof or evaluated. This
>>>> is why I considered those tests as fragile.
>>>
>>> Yes, and I think that's non-conforming.
>>>
>>
>> Good, then we have something to fix.
>>
>>> As I said in the other thread, I'd really like to see references to
>>> the standard used to justify any changes to our istreambuf_iterator
>>
>> I think _M_c has been introduced to cover this paragraph of the
>> Standard:
>>
>> 24.6.3.1
>>
>> "1. Class istreambuf_iterator<charT,traits>::proxy is for exposition
>> only. An implementation is permit-
>> ted to provide equivalent functionality without providing a class
>> with this name. Class istreambuf_-
>> iterator<charT, traits>::proxy provides a temporary placeholder as
>> the return value of the post-
>> increment operator (operator++). It keeps the character pointed to by
>> the previous value of the iterator for
>> some possible future access to get the character."
>>
>> This is why it is being set in operator++(int):
>>
>> Â Â Â istreambuf_iterator __old = *this;
>> Â Â Â __old._M_c = _M_sbuf->sbumpc();
>>
>> It is also the reason why libstdc++ fails:
>> http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp
>>
>>
>>
>> It looks like this test is simply not Standard conformant.
>
> I think you're right, because it relies on a property that may not be
> true:
>
> Â any copies of the previous
> Â value of r are no longer
> Â required either to be
> Â dereferenceable or to be in
> Â the domain of ==.
>
>> I guess at some point _M_c started to be used also to cache the
>> streambuf::sgetc resulting in current situation.
>
> And arguably that is not "equivalent functionality" to returning a
> proxy object. Although the standard seems a bit underspecified.
>
>> In attached patch I limit usage of _M_c to cover 24.6.3.1.1 point and
>> so made additional changes to 2.cc test case to demonstrate it.
>
> Doesn't the modified test PASS anyway, even without changing how _M_c
> is used?
No it doesn't because the first check for eof capture the 'a' char which
is never reseted so after string construction it is still pointing to
this char and is not an eof iterator. Without the _M_c assignment the
iterator is still "live" and so keeps on reflecting the streambuf state.
I'll show you some other side effects of this _M_c later.
>
> Generally this looks good (I'm not sure if it addresses Petr's
> concerns, but I don't think his "stop and go" usage is valid anyway -
> it's certainly not portable or guaranteed by the standard).
No it doesn't address Petr's concern who is more focus on the _M_sbuf
nullification part.
>
>> @@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> Â Â Â Â Â : _M_sbuf(0), _M_c(traits_type::eof()) { }
>>
>> #if __cplusplus >= 201103L
>> -Â Â Â Â Â istreambuf_iterator(const istreambuf_iterator&) noexcept =
>> default;
>> +Â Â Â Â Â istreambuf_iterator(const istreambuf_iterator&) = default;
>
> Please don't remove this.
>
> It's present in the standard, and it ensures we don't accidentally add
> a member that makes the default noexcept(false).
Ok
>
>> )),
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â _M_message(__gnu_debug::__msg_inc_istreambuf)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ._M_iterator(*this));
>> -Â Â Â if (_M_sbuf)
>> -Â Â Â Â Â {
>
> This check should be unnecessary, because it's undefined to increment
> and end-of-stream iterator, but I wonder if anyone is relying on it
> being currently a no-op, rather than crashing.
>
> Let's remove the check, and if it causes too many problems we can
> restore it as:
>
> Â if (__builtin_expect(_M_sbuf != 0, 1))
>
>> -Â Â Â Â Â Â Â else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â __eof))
>> -Â Â Â Â Â Â Â Â Â _M_c = __ret;
>
> Removing this assignment means that we will call _M_sbuf->sgetc() for
> every dereference operation, instead of caching the current value in
> _M_c. I think that's probably OK, because it's unlikely that
> performance critical code is dereferencing the same iterator
> repeatedly without incrementing it.
>
> And getting rid of the mutable member is a Good Thing.
>
> An alternative would be to cache it in operator*() instead of in
> _M_get(), which would still give the desired change of not updating it
> when _M_get() is used elsewhere. But would still involve setting a
> mutable member in a const function.
>
>> -Â Â Â Â Â Â Â else
>> +Â Â Â int_type __ret = _M_c;
>> +Â Â Â if (_M_sbuf && _S_at_eof(__ret) && _S_at_eof(__ret =
>> _M_sbuf->sgetc()))
>> Â Â Â Â Â _M_sbuf = 0;
>
> It's unfortunate that we still have this assignment to a mutable
> member in a const function, but I think it's necessary to avoid an
> end-of-stream iterator becoming valid again.
Yes, an alternative could be to reset it in increment operators but it
had other side effects. I'll try to send you the result of this approach.
>
>> -Â Â Â Â Â }
>> Â Â Â Â return __ret;
>> Â Â Â Â Â }
>>
>> Â Â Â Â Â bool
>> Â Â Â Â Â _M_at_eof() const
>> +Â Â Â Â Â { return _S_at_eof(_M_get()); }
>> +
>> +Â Â Â Â Â static bool
>> +Â Â Â Â Â _S_at_eof(int_type __c)
>
> This should be called _S_is_eof, since it tests if the argument **is**
> the EOF character. It doesn't test if the stream is **at** EOF, like
> _M_at_eof does.
>
> So with a suitable ChangeLog, the "noexcept" kept on the constructor,
> and renaming _S_at_eof to _S_is_eof, this is OK for trunk
Attached patch committed with:
2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>
      François Dumont <fdumont@gcc.gnu.org>
   * include/bits/streambuf_iterator.h
   (istreambuf_iterator<>::operator*()): Do not capture iterator state
   in Debug assertion.
   (istreambuf_iterator<>::operator++()): Likewise and remove _M_sbuf
check.
   (istreambuf_iterator<>::operator++(int)): Likewise.
   (istreambuf_iterator<>::_M_get()): Remove _M_c assignment.
   (istreambuf_iterator<>::_S_is_eof()): New.
   (istreambuf_iterator<>::_M_at_eof()): Adapt, use latter.
   (find(istreambuf_iterator<>, istreambuf_iterator<>, _CharT)):
   Return an iterator with _M_c set to eof to capture streambuf state
   on evaluation.
   (testsuite/24_iterators/istreambuf_iterator/2.cc): Add checks.
François
[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5057 bytes --]
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..64b8cfd 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,7 +95,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// NB: This implementation assumes the "end of stream" value
// is EOF, or -1.
mutable streambuf_type* _M_sbuf;
- mutable int_type _M_c;
+ int_type _M_c;
public:
/// Construct end of input stream iterator.
@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
char_type
operator*() const
{
+ int_type __c = _M_get();
+
#ifdef _GLIBCXX_DEBUG_PEDANTIC
// Dereferencing a past-the-end istreambuf_iterator is a
// libstdc++ extension
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(!_S_is_eof(__c),
_M_message(__gnu_debug::__msg_deref_istreambuf)
._M_iterator(*this));
#endif
- return traits_type::to_char_type(_M_get());
+ return traits_type::to_char_type(__c);
}
/// Advance the iterator. Calls streambuf.sbumpc().
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf &&
+ (!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
- if (_M_sbuf)
- {
- _M_sbuf->sbumpc();
- _M_c = traits_type::eof();
- }
+
+ _M_sbuf->sbumpc();
+ _M_c = traits_type::eof();
return *this;
}
@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf &&
+ (!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
istreambuf_iterator __old = *this;
- if (_M_sbuf)
- {
- __old._M_c = _M_sbuf->sbumpc();
- _M_c = traits_type::eof();
- }
+ __old._M_c = _M_sbuf->sbumpc();
+ _M_c = traits_type::eof();
return __old;
}
@@ -176,26 +175,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
int_type
_M_get() const
{
- const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
- if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
- _M_sbuf = 0;
- }
+ int_type __ret = _M_c;
+ if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
+ _M_sbuf = 0;
return __ret;
}
bool
_M_at_eof() const
+ { return _S_is_eof(_M_get()); }
+
+ static bool
+ _S_is_eof(int_type __c)
{
const int_type __eof = traits_type::eof();
- return traits_type::eq_int_type(_M_get(), __eof);
+ return traits_type::eq_int_type(__c, __eof);
}
};
@@ -373,13 +367,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::traits_type traits_type;
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
+ const int_type __eof = traits_type::eof();
if (__first._M_sbuf && !__last._M_sbuf)
{
const int_type __ival = traits_type::to_int_type(__val);
streambuf_type* __sb = __first._M_sbuf;
int_type __c = __sb->sgetc();
- while (!traits_type::eq_int_type(__c, traits_type::eof())
+ while (!traits_type::eq_int_type(__c, __eof)
&& !traits_type::eq_int_type(__c, __ival))
{
streamsize __n = __sb->egptr() - __sb->gptr();
@@ -396,11 +391,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__c = __sb->snextc();
}
- if (!traits_type::eq_int_type(__c, traits_type::eof()))
- __first._M_c = __c;
- else
- __first._M_sbuf = 0;
+ __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 b81f4d4..572ea9f 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
void test02(void)
{
-
typedef std::istreambuf_iterator<char> cistreambuf_iter;
- typedef cistreambuf_iter::streambuf_type cstreambuf_type;
const char slit01[] = "playa hermosa, liberia, guanacaste";
std::string str01(slit01);
std::istringstream istrs00(str01);
@@ -35,10 +33,14 @@ void test02(void)
// ctor sanity checks
cistreambuf_iter istrb_it01(istrs00);
- cistreambuf_iter istrb_it02;
- std::string tmp(istrb_it01, istrb_it02);
+ cistreambuf_iter istrb_eos;
+ VERIFY( istrb_it01 != istrb_eos );
+
+ std::string tmp(istrb_it01, istrb_eos);
VERIFY( tmp == str01 );
+ VERIFY( istrb_it01 == istrb_eos );
+
cistreambuf_iter istrb_it03(0);
cistreambuf_iter istrb_it04;
VERIFY( istrb_it03 == istrb_it04 );
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make tests less istreambuf_iterator implementation dependent
2017-10-04 16:21 ` François Dumont
@ 2017-10-04 23:23 ` Jonathan Wakely
2017-11-15 20:54 ` [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" Petr Ovtchenkov
1 sibling, 0 replies; 55+ messages in thread
From: Jonathan Wakely @ 2017-10-04 23:23 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On 04/10/17 18:21 +0200, François Dumont wrote:
>On 03/10/2017 16:20, Jonathan Wakely wrote:
>>Doesn't the modified test PASS anyway, even without changing how _M_c
>>is used?
>
>No it doesn't because the first check for eof capture the 'a' char
>which is never reseted so after string construction it is still
>pointing to this char and is not an eof iterator. Without the _M_c
>assignment the iterator is still "live" and so keeps on reflecting the
>streambuf state.
Ah yes, I must have not adjusted the test properly when I tried to
check that. Your new test does indeed fail with the old
implementation.
>Attached patch committed with:
Thanks.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf)
2017-10-03 20:39 ` Petr Ovtchenkov
2017-10-04 5:04 ` [PATCH v2] " Petr Ovtchenkov
@ 2017-10-06 16:01 ` François Dumont
2017-10-06 18:03 ` Petr Ovtchenkov
2017-10-10 14:22 ` [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf) Jonathan Wakely
1 sibling, 2 replies; 55+ messages in thread
From: François Dumont @ 2017-10-06 16:01 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- 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;
}
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf)
2017-10-06 16:01 ` [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf) François Dumont
@ 2017-10-06 18:03 ` Petr Ovtchenkov
2017-10-08 15:45 ` [PATCH] libstdc++: istreambuf_iterator proxy François Dumont
2017-10-10 14:22 ` [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf) Jonathan Wakely
1 sibling, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-10-06 18:03 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On Fri, 6 Oct 2017 18:01:36 +0200
François Dumont <frs.dumont@gmail.com> wrote:
> ...
> >>> 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 ?
It's strange question. Because EOL (end-of-life) for istreambuf_iterator
object after EOF of associated streambuf
- not directly follow from standard, just follow from (IMO wrong)
implementations; and this is a balk for useful usage of istreambuf_iterator;
- violate expectations from point of view of C++ objects life cycle;
- require destruction and construction of istreambuf_iterator
object after check for equality with istreambuf_iterator()
(strange trick).
>
> > We have three issues with istreambuf_iterator:
> > - debug-dependent behaviour
> Fixed.
+ __glibcxx_requires_cond(_M_sbuf,
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
vs
+ __glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
and
+ __glibcxx_requires_cond(_M_sbuf
+ && !traits_type::eq_int_type(_M_c,traits_type::eof()),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
vs
+ __glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
I'm insist on the first variant. It free from code that may lead to different
behaviour between debug/non-debug (like _M_sbuf->sgetc()).
> > - 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" );
No. I mean that in last (my) suggestion ([PATCH])
std::istreambuf_iterator<char> i = j++;
is impossible ("impossible" is in aggree with standard).
So test test01() in 2.cc isn't correct.
>
> 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
>
--
- ptr
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator proxy
2017-10-06 18:03 ` Petr Ovtchenkov
@ 2017-10-08 15:45 ` François Dumont
2017-10-09 19:35 ` Petr Ovtchenkov
2017-10-10 5:52 ` Petr Ovtchenkov
0 siblings, 2 replies; 55+ messages in thread
From: François Dumont @ 2017-10-08 15:45 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
On 06/10/2017 20:00, Petr Ovtchenkov wrote:
> On Fri, 6 Oct 2017 18:01:36 +0200
> François Dumont <frs.dumont@gmail.com> wrote:
>
>> ...
>>>>> 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 ?
> It's strange question. Because EOL (end-of-life) for istreambuf_iterator
> object after EOF of associated streambuf
>
> - not directly follow from standard, just follow from (IMO wrong)
> implementations; and this is a balk for useful usage of istreambuf_iterator;
> - violate expectations from point of view of C++ objects life cycle;
> - require destruction and construction of istreambuf_iterator
> object after check for equality with istreambuf_iterator()
> (strange trick).
>
>>> We have three issues with istreambuf_iterator:
>>> - debug-dependent behaviour
>> Fixed.
> + __glibcxx_requires_cond(_M_sbuf,
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
> vs
>
> + __glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
>
> and
>
> + __glibcxx_requires_cond(_M_sbuf
> + && !traits_type::eq_int_type(_M_c,traits_type::eof()),
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
> vs
>
> + __glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
> _M_message(__gnu_debug::__msg_inc_istreambuf)
> ._M_iterator(*this));
>
> I'm insist on the first variant. It free from code that may lead to different
> behaviour between debug/non-debug (like _M_sbuf->sgetc()).
The first patch fixed the impact of the Debug mode on the
istreambuf_iterator state. This kind of difference is classical with
Debug mode, this mode usually introduces additional calls to some
operators or in this case to sgetc.
We have no choice here as otherwise we wouldn't detect that the iterator
is an eof one. I might propose another patch after this one that could
allow to limit the check to _M_sbuf not being null.
>>> - 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" );
> No. I mean that in last (my) suggestion ([PATCH])
>
> std::istreambuf_iterator<char> i = j++;
>
> is impossible ("impossible" is in aggree with standard).
> So test test01() in 2.cc isn't correct.
It is correct as this constructor:
+Â Â Â Â Â /// Construct start of streambuf iterator.
+Â Â Â Â Â istreambuf_iterator(const proxy& __prx) _GLIBCXX_USE_NOEXCEPT
+Â Â Â Â Â : _M_sbuf(__prx._M_sbuf)
+Â Â Â Â Â { }
is defined by the Standard. This is why the llvm test is fine too.
What is illegal is rather something like:
++(j++);
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator proxy
2017-10-08 15:45 ` [PATCH] libstdc++: istreambuf_iterator proxy François Dumont
@ 2017-10-09 19:35 ` Petr Ovtchenkov
2017-10-10 5:52 ` Petr Ovtchenkov
1 sibling, 0 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-10-09 19:35 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On Sun, 8 Oct 2017 16:59:39 +0200
François Dumont <frs.dumont@gmail.com> wrote:
> ...
> >>
> >> 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" );
> > No. I mean that in last (my) suggestion ([PATCH])
> >
> > std::istreambuf_iterator<char> i = j++;
> >
> > is impossible ("impossible" is in aggree with standard).
> > So test test01() in 2.cc isn't correct.
>
> It is correct as this constructor:
> + /// Construct start of streambuf iterator.
> + istreambuf_iterator(const proxy& __prx) _GLIBCXX_USE_NOEXCEPT
> + : _M_sbuf(__prx._M_sbuf)
> + { }
>
> is defined by the Standard. This is why the llvm test is fine too.
Yes, you right here.
But in
+ 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' );
the last two lines
+ VERIFY( *i == 'b' );
+ VERIFY( *j++ == 'b' );
is a check of the implementation-specific behaviour, because of
++r ... 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
(i is a copy of "previous" value of j)
From other side, for ctor from proxy
istreambuf_iterator(const proxy& p) noexcept;
5 Effects: Initializes sbuf_ with p.sbuf_.
that mean that reference of istreambuf_iterator to basic_streambuf
(sbuf_) has some meaning. proxy may be dereferenced or used
to produce istreambuf_iterator that may be used for == (but not
for dereference itself ;)).
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator proxy
2017-10-08 15:45 ` [PATCH] libstdc++: istreambuf_iterator proxy François Dumont
2017-10-09 19:35 ` Petr Ovtchenkov
@ 2017-10-10 5:52 ` Petr Ovtchenkov
1 sibling, 0 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-10-10 5:52 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On Sun, 8 Oct 2017 16:59:39 +0200
François Dumont <frs.dumont@gmail.com> wrote:
> ...
> >>> We have three issues with istreambuf_iterator:
> >>> - debug-dependent behaviour
> >> Fixed.
> > + __glibcxx_requires_cond(_M_sbuf,
> > _M_message(__gnu_debug::__msg_inc_istreambuf)
> > ._M_iterator(*this));
> > vs
> >
> > + __glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
> > _M_message(__gnu_debug::__msg_inc_istreambuf)
> > ._M_iterator(*this));
> >
> > and
> >
> > + __glibcxx_requires_cond(_M_sbuf
> > + && !traits_type::eq_int_type(_M_c,traits_type::eof()),
> > _M_message(__gnu_debug::__msg_inc_istreambuf)
> > ._M_iterator(*this));
> > vs
> >
> > + __glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
> > _M_message(__gnu_debug::__msg_inc_istreambuf)
> > ._M_iterator(*this));
> >
> > I'm insist on the first variant. It free from code that may lead to different
> > behaviour between debug/non-debug (like _M_sbuf->sgetc()).
>
> The first patch fixed the impact of the Debug mode on the
> istreambuf_iterator state. This kind of difference is classical with
> Debug mode, this mode usually introduces additional calls to some
> operators or in this case to sgetc.
>
The key is "_M_sbuf->sgetc()" and "_S_is_eof" that may call ->sgetc() too.
This may lead to difference in debug/non-debug behaviour.
Solution without such difference exist and was presented here.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf)
2017-10-06 16:01 ` [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf) François Dumont
2017-10-06 18:03 ` Petr Ovtchenkov
@ 2017-10-10 14:22 ` Jonathan Wakely
1 sibling, 0 replies; 55+ messages in thread
From: Jonathan Wakely @ 2017-10-10 14:22 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On 06/10/17 18:01 +0200, François Dumont wrote:
>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'.
Right, but this code isn't required to work. These are Input
Iterators. The fact that incrementing j affects other copies of the
iterator is expected.
>We could improve the situation by adding a debug assertion that _M_c
>is eof when pre-increment is being used
Hmm, interesting idea.
>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*().
I agree with the earlier analysis that the libc++ test is not required
to work (i've reported this to the libc++ maintainers and expect them
to move it to their nonportable test directory). So we don't need to
change our implementation to make it pass.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Make istreambuf_iterator::_M_sbuf immutable and add debug checks
@ 2017-10-13 17:22 François Dumont
2017-10-23 19:24 ` François Dumont
0 siblings, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-10-13 17:22 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 774 bytes --]
Hi
   Here is the last patch I will propose for istreambuf_iterator.
This is mostly to remove the mutable keyword on _M_sbuf.
   To do so I had to reset _M_sbuf in valid places that is to say
constructors and increment operators. Despite that we might still have
eof iterators with _M_sbuf not null when you have for instance several
iterator instance but only increment one. It seems fine to me because
even in this case iterator will still be considered as eof and using
several istreambuf_iterator to go through a given streambuf is not usual.
   As _M_sbuf is immutable I have been able to restore the simple
call to _M_at_eof() in the increment operators debug check.
Ok to commit after successful tests ?
François
[-- Attachment #2: immutable_streambuf_ite.patch --]
[-- Type: text/x-patch, Size: 5861 bytes --]
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 081afe5..ef22aa9 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,8 +94,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// the "end of stream" iterator value.
// NB: This implementation assumes the "end of stream" value
// is EOF, or -1.
- mutable streambuf_type* _M_sbuf;
- int_type _M_c;
+ streambuf_type* _M_sbuf;
+ int_type _M_c;
public:
/// Construct end of input stream iterator.
@@ -110,11 +110,13 @@ _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()), _M_c(traits_type::eof())
+ { _M_init(); }
/// Construct start of streambuf iterator.
istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
- : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+ : _M_sbuf(__s), _M_c(traits_type::eof())
+ { _M_init(); }
/// Return the current character pointed to by iterator. This returns
/// streambuf.sgetc(). It cannot be assigned. NB: The result of
@@ -138,13 +140,16 @@ _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_at_eof(),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
_M_sbuf->sbumpc();
_M_c = traits_type::eof();
+
+ if (_S_is_eof(_M_sbuf->sgetc()))
+ _M_sbuf = 0;
+
return *this;
}
@@ -152,14 +157,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(_M_sbuf &&
- (!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+ __glibcxx_requires_cond(!_M_at_eof(),
_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();
+
+ if (_S_is_eof(_M_sbuf->sgetc()))
+ _M_sbuf = __old._M_sbuf = 0;
+
return __old;
}
@@ -172,12 +180,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
{ return _M_at_eof() == __b._M_at_eof(); }
private:
+ void
+ _M_init()
+ {
+ if (_M_sbuf && _S_is_eof(_M_sbuf->sgetc()))
+ _M_sbuf = 0;
+ }
+
int_type
_M_get() const
{
int_type __ret = _M_c;
- if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
- _M_sbuf = 0;
+ if (_M_sbuf && __builtin_expect(_S_is_eof(__ret), true))
+ __ret = _M_sbuf->sgetc();
+
return __ret;
}
@@ -391,10 +407,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__c = __sb->snextc();
}
- __first._M_c = __eof;
+ if (!traits_type::eq_int_type(__c, __eof))
+ {
+ __first._M_c = __eof;
+ return __first;
+ }
}
- return __first;
+ return __last;
}
// @} group iterators
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
new file mode 100644
index 0000000..241fc58
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+ typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+ cistreambuf_iter eof;
+ ++eof; // Invalid.
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
new file mode 100644
index 0000000..407f00b
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+ typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+ cistreambuf_iter eof;
+ eof++; // Invalid.
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks
2017-10-13 17:22 Make istreambuf_iterator::_M_sbuf immutable and add debug checks François Dumont
@ 2017-10-23 19:24 ` François Dumont
2017-11-06 21:20 ` François Dumont
0 siblings, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-10-23 19:24 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]
Hi
   I completed execution of all tests and found one test impacted by
this patch.
   It is a good example of the impact of the patch. Users won't be
able to build a istreambuf_iterator at a point where the underlying
streambuf is at end-of-stream and then put some data in the streambuf
and then use the iterator. This is similar to what Petr was proposing,
some eof iterator becoming valid again through an operation on the
streambuf. I would prefer we forbid it completely or we accept it
completely but current middle way situation is strange.
   The fix is easy, let the compiler build the streambuf_iterator
when needed. Even if patch is not accepted I think we should keep the
change on the test which is fragile.
François
On 13/10/2017 19:14, François Dumont wrote:
> Hi
>
> Â Â Â Â Here is the last patch I will propose for istreambuf_iterator.
> This is mostly to remove the mutable keyword on _M_sbuf.
>
> Â Â Â Â To do so I had to reset _M_sbuf in valid places that is to say
> constructors and increment operators. Despite that we might still have
> eof iterators with _M_sbuf not null when you have for instance several
> iterator instance but only increment one. It seems fine to me because
> even in this case iterator will still be considered as eof and using
> several istreambuf_iterator to go through a given streambuf is not usual.
>
> Â Â Â Â As _M_sbuf is immutable I have been able to restore the simple
> call to _M_at_eof() in the increment operators debug check.
>
> Ok to commit after successful tests ?
>
> François
>
>
>
[-- Attachment #2: immutable_streambuf_ite.patch --]
[-- Type: text/x-patch, Size: 9704 bytes --]
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 081afe5..0a6c7f9 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,7 +94,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// the "end of stream" iterator value.
// NB: This implementation assumes the "end of stream" value
// is EOF, or -1.
- mutable streambuf_type* _M_sbuf;
+ streambuf_type* _M_sbuf;
int_type _M_c;
public:
@@ -110,11 +110,13 @@ _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()), _M_c(traits_type::eof())
+ { _M_init(); }
/// Construct start of streambuf iterator.
istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
- : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+ : _M_sbuf(__s), _M_c(traits_type::eof())
+ { _M_init(); }
/// Return the current character pointed to by iterator. This returns
/// streambuf.sgetc(). It cannot be assigned. NB: The result of
@@ -138,13 +140,16 @@ _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_at_eof(),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
_M_sbuf->sbumpc();
_M_c = traits_type::eof();
+
+ if (_S_is_eof(_M_sbuf->sgetc()))
+ _M_sbuf = 0;
+
return *this;
}
@@ -152,14 +157,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(_M_sbuf &&
- (!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+ __glibcxx_requires_cond(!_M_at_eof(),
_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();
+
+ if (_S_is_eof(_M_sbuf->sgetc()))
+ _M_sbuf = __old._M_sbuf = 0;
+
return __old;
}
@@ -172,12 +180,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
{ return _M_at_eof() == __b._M_at_eof(); }
private:
+ void
+ _M_init()
+ {
+ if (_M_sbuf && _S_is_eof(_M_sbuf->sgetc()))
+ _M_sbuf = 0;
+ }
+
int_type
_M_get() const
{
int_type __ret = _M_c;
- if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
- _M_sbuf = 0;
+ if (_M_sbuf && __builtin_expect(_S_is_eof(__ret), true))
+ __ret = _M_sbuf->sgetc();
+
return __ret;
}
@@ -391,10 +407,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__c = __sb->snextc();
}
+ if (!traits_type::eq_int_type(__c, __eof))
+ {
__first._M_c = __eof;
+ return __first;
+ }
}
- return __first;
+ return __last;
}
// @} group iterators
diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc
index 9b69956..476e38f 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc
@@ -41,7 +41,6 @@ int main()
= std::use_facet<std::money_get<char> >(liffey.getloc());
typedef std::istreambuf_iterator<char> iterator_type;
- iterator_type is(liffey);
iterator_type end;
std::ios_base::iostate err01 = std::ios_base::goodbit;
@@ -50,7 +49,7 @@ int main()
// Feed it 1 digit too many, which should fail.
liffey.str("12.3456");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if (! (err01 & std::ios_base::failbit ))
fails |= 0x01;
@@ -58,7 +57,7 @@ int main()
// Feed it exactly what it wants, which should succeed.
liffey.str("12.345");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if ( err01 & std::ios_base::failbit )
fails |= 0x02;
@@ -66,7 +65,7 @@ int main()
// Feed it 1 digit too few, which should fail.
liffey.str("12.34");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if (! ( err01 & std::ios_base::failbit ))
fails |= 0x04;
@@ -74,7 +73,7 @@ int main()
// Feed it only a decimal-point, which should fail.
liffey.str("12.");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if (! (err01 & std::ios_base::failbit ))
fails |= 0x08;
@@ -82,7 +81,7 @@ int main()
// Feed it no decimal-point at all, which should succeed.
liffey.str("12");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if ( err01 & std::ios_base::failbit )
fails |= 0x10;
diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc
index a08a713..e5f8def 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc
@@ -41,7 +41,6 @@ int main()
= std::use_facet<std::money_get<wchar_t> >(liffey.getloc());
typedef std::istreambuf_iterator<wchar_t> iterator_type;
- iterator_type is(liffey);
iterator_type end;
std::ios_base::iostate err01 = std::ios_base::goodbit;
@@ -50,7 +49,7 @@ int main()
// Feed it 1 digit too many, which should fail.
liffey.str(L"12.3456");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if (! (err01 & std::ios_base::failbit ))
fails |= 0x01;
@@ -58,7 +57,7 @@ int main()
// Feed it exactly what it wants, which should succeed.
liffey.str(L"12.345");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if ( err01 & std::ios_base::failbit )
fails |= 0x02;
@@ -66,7 +65,7 @@ int main()
// Feed it 1 digit too few, which should fail.
liffey.str(L"12.34");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if (! ( err01 & std::ios_base::failbit ))
fails |= 0x04;
@@ -74,7 +73,7 @@ int main()
// Feed it only a decimal-point, which should fail.
liffey.str(L"12.");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if (! (err01 & std::ios_base::failbit ))
fails |= 0x08;
@@ -82,7 +81,7 @@ int main()
// Feed it no decimal-point at all, which should succeed.
liffey.str(L"12");
- greed.get(is, end, false, liffey, err01, coins);
+ greed.get(liffey, end, false, liffey, err01, coins);
if ( err01 & std::ios_base::failbit )
fails |= 0x10;
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
new file mode 100644
index 0000000..241fc58
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+ typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+ cistreambuf_iter eof;
+ ++eof; // Invalid.
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
new file mode 100644
index 0000000..407f00b
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+ typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+ cistreambuf_iter eof;
+ eof++; // Invalid.
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks
2017-10-23 19:24 ` François Dumont
@ 2017-11-06 21:20 ` François Dumont
2017-11-16 6:07 ` Petr Ovtchenkov
0 siblings, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-11-06 21:20 UTC (permalink / raw)
To: libstdc++, gcc-patches
Hi
   Any final decision regarding this patch ?
François
On 23/10/2017 21:08, François Dumont wrote:
> Hi
>
> Â Â Â Â I completed execution of all tests and found one test impacted by
> this patch.
>
> Â Â Â Â It is a good example of the impact of the patch. Users won't be
> able to build a istreambuf_iterator at a point where the underlying
> streambuf is at end-of-stream and then put some data in the streambuf
> and then use the iterator. This is similar to what Petr was proposing,
> some eof iterator becoming valid again through an operation on the
> streambuf. I would prefer we forbid it completely or we accept it
> completely but current middle way situation is strange.
>
> Â Â Â Â The fix is easy, let the compiler build the streambuf_iterator
> when needed. Even if patch is not accepted I think we should keep the
> change on the test which is fragile.
>
> François
>
>
> On 13/10/2017 19:14, François Dumont wrote:
>> Hi
>>
>> Â Â Â Â Here is the last patch I will propose for istreambuf_iterator.
>> This is mostly to remove the mutable keyword on _M_sbuf.
>>
>> Â Â Â Â To do so I had to reset _M_sbuf in valid places that is to say
>> constructors and increment operators. Despite that we might still
>> have eof iterators with _M_sbuf not null when you have for instance
>> several iterator instance but only increment one. It seems fine to me
>> because even in this case iterator will still be considered as eof
>> and using several istreambuf_iterator to go through a given streambuf
>> is not usual.
>>
>> Â Â Â Â As _M_sbuf is immutable I have been able to restore the simple
>> call to _M_at_eof() in the increment operators debug check.
>>
>> Ok to commit after successful tests ?
>>
>> François
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 4/4] libstdc++: immutable _M_sbuf in istreambuf_iterator
2017-11-15 20:52 ` [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator Petr Ovtchenkov
@ 2017-11-15 20:52 ` Petr Ovtchenkov
2017-11-15 22:30 ` [PATCH 3/4] libstdc++: avoid character accumulation " Paolo Carlini
1 sibling, 0 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-15 20:52 UTC (permalink / raw)
To: libstdc++; +Cc: gcc-patches
No needs to have mutable _M_sbuf in istreambuf_iterator
more.
---
libstdc++-v3/include/bits/streambuf_iterator.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 203da9d..e2b6707 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,7 +94,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// the "end of stream" iterator value.
// NB: This implementation assumes the "end of stream" value
// is EOF, or -1.
- mutable streambuf_type* _M_sbuf;
+ streambuf_type* _M_sbuf;
public:
class proxy
--
2.10.1
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator
2017-11-15 20:58 ` [PATCH 2/4] libstdc++: istreambuf_iterator keep attached streambuf Petr Ovtchenkov
@ 2017-11-15 20:52 ` Petr Ovtchenkov
2017-11-15 20:52 ` [PATCH 4/4] libstdc++: immutable _M_sbuf " Petr Ovtchenkov
2017-11-15 22:30 ` [PATCH 3/4] libstdc++: avoid character accumulation " Paolo Carlini
0 siblings, 2 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-15 20:52 UTC (permalink / raw)
To: libstdc++; +Cc: gcc-patches
Ask associated streambuf for character when needed instead of
accumulate it in istreambuf_iterator object.
Benefits from this:
- minus one class member in istreambuf_iterator
- trivial synchronization of states of istreambuf_iterator
and associated streambuf
---
libstdc++-v3/include/bits/streambuf_iterator.h | 34 ++++++++++++--------------
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 08fb13b..203da9d 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,19 +95,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// NB: This implementation assumes the "end of stream" value
// is EOF, or -1.
mutable streambuf_type* _M_sbuf;
- mutable int_type _M_c;
public:
class proxy
{
friend class istreambuf_iterator;
private:
- proxy(int_type c, streambuf_type* sbuf_) :
+ proxy(int_type c, streambuf_type* sbuf_) :
_M_c(c),
_M_sbuf(sbuf_)
{ }
int_type _M_c;
- streambuf_type* _M_sbuf;
+ streambuf_type* _M_sbuf;
public:
char_type
@@ -118,7 +117,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
public:
/// Construct end of input stream iterator.
_GLIBCXX_CONSTEXPR istreambuf_iterator() _GLIBCXX_USE_NOEXCEPT
- : _M_sbuf(0), _M_c(traits_type::eof()) { }
+ : _M_sbuf(0) { }
#if __cplusplus >= 201103L
istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
@@ -128,15 +127,15 @@ _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) { }
/// Construct start of istreambuf iterator.
istreambuf_iterator(const proxy& __p) _GLIBCXX_USE_NOEXCEPT
- : _M_sbuf(__p._M_sbuf), _M_c(traits_type::eof()) { }
+ : _M_sbuf(__p._M_sbuf) { }
/// Return the current character pointed to by iterator. This returns
/// streambuf.sgetc(). It cannot be assigned. NB: The result of
@@ -147,11 +146,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
#ifdef _GLIBCXX_DEBUG_PEDANTIC
// Dereferencing a past-the-end istreambuf_iterator is a
// libstdc++ extension
- __glibcxx_requires_cond(!_M_at_eof(),
+ int_type __tmp = _M_get();
+ __glibcxx_requires_cond(!traits_type::eq_int_type(__tmp,traits_type::eof()),
_M_message(__gnu_debug::__msg_deref_istreambuf)
._M_iterator(*this));
-#endif
+ return traits_type::to_char_type(__tmp);
+#else
return traits_type::to_char_type(_M_get());
+#endif
}
/// Advance the iterator. Calls streambuf.sbumpc().
@@ -172,7 +174,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
#endif
- _M_c = traits_type::eof();
}
return *this;
}
@@ -181,17 +182,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
proxy
operator++(int)
{
- _M_get();
- __glibcxx_requires_cond(_M_sbuf
- && !traits_type::eq_int_type(_M_c,traits_type::eof()),
+ int_type c = _M_get();
+ __glibcxx_requires_cond(!traits_type::eq_int_type(c,traits_type::eof()),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
- proxy __old(_M_c, _M_sbuf);
+ proxy __old(c, _M_sbuf);
if (_M_sbuf)
{
_M_sbuf->sbumpc();
- _M_c = traits_type::eof();
}
return __old;
}
@@ -209,9 +208,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_get() const
{
const int_type __eof = traits_type::eof();
- if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
- _M_c = _M_sbuf->sgetc();
- return _M_c;
+ return _M_sbuf ? _M_sbuf->sgetc() : __eof;
}
bool
@@ -418,7 +415,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
else
__c = __sb->snextc();
}
- __first._M_c = __c;
}
return __first;
}
--
2.10.1
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"
2017-10-04 16:21 ` François Dumont
2017-10-04 23:23 ` Jonathan Wakely
@ 2017-11-15 20:54 ` Petr Ovtchenkov
2017-11-15 20:58 ` [PATCH 2/4] libstdc++: istreambuf_iterator keep attached streambuf Petr Ovtchenkov
2017-11-16 11:03 ` [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" Jonathan Wakely
1 sibling, 2 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-15 20:54 UTC (permalink / raw)
To: libstdc++; +Cc: gcc-patches
This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
(svn r253417).
---
libstdc++-v3/include/bits/streambuf_iterator.h | 59 ++++++++++++++------------
1 file changed, 33 insertions(+), 26 deletions(-)
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 081afe5..69ee013 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,7 +95,7 @@ _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;
+ mutable int_type _M_c;
public:
/// Construct end of input stream iterator.
@@ -122,29 +122,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
char_type
operator*() const
{
- int_type __c = _M_get();
-
#ifdef _GLIBCXX_DEBUG_PEDANTIC
// Dereferencing a past-the-end istreambuf_iterator is a
// libstdc++ extension
- __glibcxx_requires_cond(!_S_is_eof(__c),
+ __glibcxx_requires_cond(!_M_at_eof(),
_M_message(__gnu_debug::__msg_deref_istreambuf)
._M_iterator(*this));
#endif
- return traits_type::to_char_type(__c);
+ return traits_type::to_char_type(_M_get());
}
/// Advance the iterator. Calls streambuf.sbumpc().
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(_M_sbuf &&
- (!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+ __glibcxx_requires_cond(!_M_at_eof(),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
-
- _M_sbuf->sbumpc();
- _M_c = traits_type::eof();
+ if (_M_sbuf)
+ {
+ _M_sbuf->sbumpc();
+ _M_c = traits_type::eof();
+ }
return *this;
}
@@ -152,14 +151,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(_M_sbuf &&
- (!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+ __glibcxx_requires_cond(!_M_at_eof(),
_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();
+ if (_M_sbuf)
+ {
+ __old._M_c = _M_sbuf->sbumpc();
+ _M_c = traits_type::eof();
+ }
return __old;
}
@@ -175,21 +176,26 @@ _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()))
- _M_sbuf = 0;
+ const int_type __eof = traits_type::eof();
+ int_type __ret = __eof;
+ if (_M_sbuf)
+ {
+ if (!traits_type::eq_int_type(_M_c, __eof))
+ __ret = _M_c;
+ else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
+ __eof))
+ _M_c = __ret;
+ else
+ _M_sbuf = 0;
+ }
return __ret;
}
bool
_M_at_eof() const
- { return _S_is_eof(_M_get()); }
-
- static bool
- _S_is_eof(int_type __c)
{
const int_type __eof = traits_type::eof();
- return traits_type::eq_int_type(__c, __eof);
+ return traits_type::eq_int_type(_M_get(), __eof);
}
};
@@ -367,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::traits_type traits_type;
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
- const int_type __eof = traits_type::eof();
if (__first._M_sbuf && !__last._M_sbuf)
{
const int_type __ival = traits_type::to_int_type(__val);
streambuf_type* __sb = __first._M_sbuf;
int_type __c = __sb->sgetc();
- while (!traits_type::eq_int_type(__c, __eof)
+ while (!traits_type::eq_int_type(__c, traits_type::eof())
&& !traits_type::eq_int_type(__c, __ival))
{
streamsize __n = __sb->egptr() - __sb->gptr();
@@ -391,9 +396,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__c = __sb->snextc();
}
- __first._M_c = __eof;
+ if (!traits_type::eq_int_type(__c, traits_type::eof()))
+ __first._M_c = __c;
+ else
+ __first._M_sbuf = 0;
}
-
return __first;
}
--
2.10.1
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH 2/4] libstdc++: istreambuf_iterator keep attached streambuf
2017-11-15 20:54 ` [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" Petr Ovtchenkov
@ 2017-11-15 20:58 ` Petr Ovtchenkov
2017-11-15 20:52 ` [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator Petr Ovtchenkov
2017-11-16 11:03 ` [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" Jonathan Wakely
1 sibling, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-15 20:58 UTC (permalink / raw)
To: libstdc++; +Cc: gcc-patches
istreambuf_iterator should not forget about attached
streambuf when it reach EOF.
Checks in debug mode has no infuence more on character
extraction in istreambuf_iterator increment operators.
In this aspect behaviour in debug and non-debug mode
is similar now.
Test for detached srteambuf in istreambuf_iterator:
When istreambuf_iterator reach EOF of istream, it should not
forget about attached streambuf.
From fact "EOF in stream reached" not follow that
stream reach end of life and input operation impossible
more.
postfix increment (r++) return proxy object, due to
copies of the previous value of r are no longer
required either to be dereferenceable or to be in
the domain of ==.
i.e. type that usable only for dereference and extraction
"previous" character.
istreambuf_iterator should has ctor from proxy object,
so proxy should store pointer to streambuf object.
---
libstdc++-v3/include/bits/streambuf_iterator.h | 67 ++++++++++++++--------
.../24_iterators/istreambuf_iterator/3.cc | 66 +++++++++++++++++++++
2 files changed, 109 insertions(+), 24 deletions(-)
create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 69ee013..08fb13b 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -98,6 +98,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
mutable int_type _M_c;
public:
+ class proxy
+ {
+ friend class istreambuf_iterator;
+ private:
+ proxy(int_type c, streambuf_type* sbuf_) :
+ _M_c(c),
+ _M_sbuf(sbuf_)
+ { }
+ int_type _M_c;
+ streambuf_type* _M_sbuf;
+
+ public:
+ char_type
+ operator*() const
+ { return traits_type::to_char_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()) { }
@@ -116,6 +134,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
: _M_sbuf(__s), _M_c(traits_type::eof()) { }
+ /// Construct start of istreambuf iterator.
+ istreambuf_iterator(const proxy& __p) _GLIBCXX_USE_NOEXCEPT
+ : _M_sbuf(__p._M_sbuf), _M_c(traits_type::eof()) { }
+
/// Return the current character pointed to by iterator. This returns
/// streambuf.sgetc(). It cannot be assigned. NB: The result of
/// operator*() on an end of stream is undefined.
@@ -136,29 +158,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf,
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
if (_M_sbuf)
{
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+ int_type __tmp =
+#endif
_M_sbuf->sbumpc();
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+ __glibcxx_requires_cond(!traits_type::eq_int_type(__tmp,traits_type::eof()),
+ _M_message(__gnu_debug::__msg_inc_istreambuf)
+ ._M_iterator(*this));
+#endif
_M_c = traits_type::eof();
}
return *this;
}
/// Advance the iterator. Calls streambuf.sbumpc().
- istreambuf_iterator
+ proxy
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ _M_get();
+ __glibcxx_requires_cond(_M_sbuf
+ && !traits_type::eq_int_type(_M_c,traits_type::eof()),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
- istreambuf_iterator __old = *this;
+ proxy __old(_M_c, _M_sbuf);
if (_M_sbuf)
{
- __old._M_c = _M_sbuf->sbumpc();
+ _M_sbuf->sbumpc();
_M_c = traits_type::eof();
}
return __old;
@@ -177,18 +209,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_get() const
{
const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
- if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
- _M_sbuf = 0;
- }
- return __ret;
+ if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
+ _M_c = _M_sbuf->sgetc();
+ return _M_c;
}
bool
@@ -339,7 +362,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
- if (__first._M_sbuf && !__last._M_sbuf)
+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
{
streambuf_type* __sb = __first._M_sbuf;
int_type __c = __sb->sgetc();
@@ -374,7 +397,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::streambuf_type streambuf_type;
typedef typename traits_type::int_type int_type;
- if (__first._M_sbuf && !__last._M_sbuf)
+ if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
{
const int_type __ival = traits_type::to_int_type(__val);
streambuf_type* __sb = __first._M_sbuf;
@@ -395,11 +418,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
else
__c = __sb->snextc();
}
-
- if (!traits_type::eq_int_type(__c, traits_type::eof()))
- __first._M_c = __c;
- else
- __first._M_sbuf = 0;
+ __first._M_c = __c;
}
return __first;
}
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
new file mode 100644
index 0000000..50413fa
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
@@ -0,0 +1,66 @@
+// { dg-options "-std=gnu++17" }
+
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+#include <cstring>
+#include <testsuite_hooks.h>
+
+void test03()
+{
+ using namespace std;
+
+ std::stringstream s;
+ char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
+ char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
+ char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
+ // 012345678901234567890123456789012345
+ // 0 1 2 3
+ s << b;
+ VERIFY( !s.fail() );
+
+ istreambuf_iterator<char> i(s);
+ copy_n(i, 36, r);
+ ++i; // EOF reached
+ VERIFY(i == std::istreambuf_iterator<char>());
+
+ VERIFY(memcmp(b, r, 36) == 0);
+
+ s << q;
+ VERIFY(!s.fail());
+
+ copy_n(i, 36, r);
+ ++i; // EOF reached
+ VERIFY(i == std::istreambuf_iterator<char>());
+
+ VERIFY(memcmp(q, r, 36) == 0);
+
+ s << 'Q';
+
+ VERIFY(*i++ == 'Q');
+ VERIFY(i == std::istreambuf_iterator<char>());
+}
+
+int main()
+{
+ test03();
+
+ return 0;
+}
--
2.10.1
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator
2017-11-15 20:52 ` [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator Petr Ovtchenkov
2017-11-15 20:52 ` [PATCH 4/4] libstdc++: immutable _M_sbuf " Petr Ovtchenkov
@ 2017-11-15 22:30 ` Paolo Carlini
2017-11-16 6:00 ` Petr Ovtchenkov
1 sibling, 1 reply; 55+ messages in thread
From: Paolo Carlini @ 2017-11-15 22:30 UTC (permalink / raw)
To: Petr Ovtchenkov, libstdc++; +Cc: gcc-patches
Hi,
On 15/11/2017 11:48, Petr Ovtchenkov wrote:
> Ask associated streambuf for character when needed instead of
> accumulate it in istreambuf_iterator object.
>
> Benefits from this:
> - minus one class member in istreambuf_iterator
> - trivial synchronization of states of istreambuf_iterator
> and associated streambuf
> ---
> libstdc++-v3/include/bits/streambuf_iterator.h | 34 ++++++++++++--------------
> 1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
> index 08fb13b..203da9d 100644
> --- a/libstdc++-v3/include/bits/streambuf_iterator.h
> +++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> @@ -95,19 +95,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> // NB: This implementation assumes the "end of stream" value
> // is EOF, or -1.
> mutable streambuf_type* _M_sbuf;
> - mutable int_type _M_c;
Obviously this would be an ABI-breaking change, which certainly we don't
want. Unless I missed a detailed discussion of the non-trivial way to
avoid it in one of the recent threads about these topics...
Paolo.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator
2017-11-15 22:30 ` [PATCH 3/4] libstdc++: avoid character accumulation " Paolo Carlini
@ 2017-11-16 6:00 ` Petr Ovtchenkov
2017-11-16 9:51 ` Paolo Carlini
0 siblings, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-16 6:00 UTC (permalink / raw)
To: Paolo Carlini; +Cc: libstdc++, gcc-patches
On Wed, 15 Nov 2017 22:31:11 +0100
Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 15/11/2017 11:48, Petr Ovtchenkov wrote:
> > Ask associated streambuf for character when needed instead of
> > accumulate it in istreambuf_iterator object.
> >
> > Benefits from this:
> > - minus one class member in istreambuf_iterator
> > - trivial synchronization of states of istreambuf_iterator
> > and associated streambuf
> > ---
> > libstdc++-v3/include/bits/streambuf_iterator.h | 34 ++++++++++++--------------
> > 1 file changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
> > b/libstdc++-v3/include/bits/streambuf_iterator.h index 08fb13b..203da9d 100644
> > --- a/libstdc++-v3/include/bits/streambuf_iterator.h
> > +++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> > @@ -95,19 +95,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > // NB: This implementation assumes the "end of stream" value
> > // is EOF, or -1.
> > mutable streambuf_type* _M_sbuf;
> > - mutable int_type _M_c;
> Obviously this would be an ABI-breaking change, which certainly we don't
> want. Unless I missed a detailed discussion of the non-trivial way to
> avoid it in one of the recent threads about these topics...
Is we really worry about frozen sizeof of instantiated template?
(Removed private template member).
If yes, than
int_type __dummy;
is our all.
>
> Paolo.
--
- ptr
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks
2017-11-06 21:20 ` François Dumont
@ 2017-11-16 6:07 ` Petr Ovtchenkov
2017-11-16 11:29 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-16 6:07 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
On Mon, 6 Nov 2017 22:19:22 +0100
François Dumont <frs.dumont@gmail.com> wrote:
> Hi
>
> Any final decision regarding this patch ?
>
> François
https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
--
- ptr
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator
2017-11-16 6:00 ` Petr Ovtchenkov
@ 2017-11-16 9:51 ` Paolo Carlini
2017-11-16 11:34 ` Petr Ovtchenkov
0 siblings, 1 reply; 55+ messages in thread
From: Paolo Carlini @ 2017-11-16 9:51 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
Hi,
On 16/11/2017 06:31, Petr Ovtchenkov wrote:
> Is we really worry about frozen sizeof of instantiated template?
Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
under "Prohibited Changes", point 8.
Of course removing the buffering has performance implications too -
that's why it's there in the first place! - which I remember we
investigated a bit again in the past when somebody reported that a few
implementations had it other did not. But I can't say to have followed
all the (recently uncovered) conformance implications, it could well be
that we cannot be 100% conforming to the letter of the current standard
while taking advantage of a buffering mechanism. Jonathan will provide
feedback.
Paolo.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"
2017-11-15 20:54 ` [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" Petr Ovtchenkov
2017-11-15 20:58 ` [PATCH 2/4] libstdc++: istreambuf_iterator keep attached streambuf Petr Ovtchenkov
@ 2017-11-16 11:03 ` Jonathan Wakely
2017-11-16 11:39 ` Petr Ovtchenkov
1 sibling, 1 reply; 55+ messages in thread
From: Jonathan Wakely @ 2017-11-16 11:03 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
>This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
>(svn r253417).
I'm not even going to bother to review patches sent without any
explanation or rationale for the change.
I will repeat what Paolo said: changing the ABI is not acceptable.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks
2017-11-16 6:07 ` Petr Ovtchenkov
@ 2017-11-16 11:29 ` Jonathan Wakely
2017-11-16 11:57 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: Jonathan Wakely @ 2017-11-16 11:29 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: François Dumont, libstdc++, gcc-patches
On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
>On Mon, 6 Nov 2017 22:19:22 +0100
>François Dumont <frs.dumont@gmail.com> wrote:
>
>> Hi
>>
>> Â Â Â Any final decision regarding this patch ?
>>
>> François
>
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
It would be helpful if you two could collaborate and come up with a
good solution, or at least discuss the pros and cons, instead of just
sending competing patches.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator
2017-11-16 9:51 ` Paolo Carlini
@ 2017-11-16 11:34 ` Petr Ovtchenkov
2017-11-16 11:35 ` Paolo Carlini
0 siblings, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-16 11:34 UTC (permalink / raw)
To: Paolo Carlini; +Cc: libstdc++, gcc-patches
On Thu, 16 Nov 2017 10:39:02 +0100
Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 16/11/2017 06:31, Petr Ovtchenkov wrote:
> > Is we really worry about frozen sizeof of instantiated template?
> Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
> under "Prohibited Changes", point 8.
>
> Of course removing the buffering has performance implications too -
> that's why it's there in the first place!
"buffering" here is a secondary buffering (after streambuf).
No relation to performance, but place for incoherence with
state of attached streambuf.
Of cause, I can spend time to measure the difference.
The main point of this patch series is avoidance of lost link between
streambuf and istream_iterator when istream_iterator see eof.
Implementations that forget about attached streambuf after
istream_iterator see eof (or lost synchronization with attached
streambuf) violate principles of C++ objects life cycle.
From practical point of view, such implementation block usage of
istream_iterator for sockets, ttys, etc. --- only non-modified
files remains in scope of application.
> - which I remember we
> investigated a bit again in the past when somebody reported that a few
> implementations had it other did not. But I can't say to have followed
> all the (recently uncovered) conformance implications, it could well be
> that we cannot be 100% conforming to the letter of the current standard
> while taking advantage of a buffering mechanism. Jonathan will provide
> feedback.
>
> Paolo.
--
- ptr
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator
2017-11-16 11:34 ` Petr Ovtchenkov
@ 2017-11-16 11:35 ` Paolo Carlini
2017-11-16 11:44 ` Petr Ovtchenkov
0 siblings, 1 reply; 55+ messages in thread
From: Paolo Carlini @ 2017-11-16 11:35 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
Hi,
On 16/11/2017 12:03, Petr Ovtchenkov wrote:
> On Thu, 16 Nov 2017 10:39:02 +0100
> Paolo Carlini <paolo.carlini@oracle.com> wrote:
>
>> Hi,
>>
>> On 16/11/2017 06:31, Petr Ovtchenkov wrote:
>>> Is we really worry about frozen sizeof of instantiated template?
>> Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
>> under "Prohibited Changes", point 8.
>>
>> Of course removing the buffering has performance implications too -
>> that's why it's there in the first place!
> "buffering" here is a secondary buffering (after streambuf).
> No relation to performance, but place for incoherence with
> state of attached streambuf.
It depends, we may be dealing with an unbuffered stream. For sure at the
time we measured a performance impact in some cases, likewise whoever
implemented it in the first place (not me) otherwise, again, why bothering?
Paolo.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"
2017-11-16 11:03 ` [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" Jonathan Wakely
@ 2017-11-16 11:39 ` Petr Ovtchenkov
2017-11-16 11:41 ` Jonathan Wakely
0 siblings, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-16 11:39 UTC (permalink / raw)
To: libstdc++; +Cc: Jonathan Wakely, gcc-patches
On Thu, 16 Nov 2017 10:56:29 +0000
Jonathan Wakely <jwakely@redhat.com> wrote:
> On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
> >This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
> >(svn r253417).
>
> I'm not even going to bother to review patches sent without any
> explanation or rationale for the change.
https://gcc.gnu.org/ml/libstdc++/2017-11/msg00044.html
Along with "violate principles of C++ objects life cycle",
the side-effect is
- Make istreambuf_iterator::_M_sbuf immutable
- streambuf_iterator: avoid debug-dependent behaviour
I should underline, that "_M_sbuf = 0" when istreambuf_iterator
see eof, lead to cripple lifecycle of istreambuf_iterator
object and [almost] block usage of istreambuf_iterator
for entities other then immutable files.
All tests from 24_iterators and 25_algorithms passed,
so I expect it conform to Standard.
This is series of patches, not single patch because
I keep in mind technology aspect---easy transfer
to branches other then trunk.
>
> I will repeat what Paolo said: changing the ABI is not acceptable.
I will repeat special for you:
<snip>
Is we really worry about frozen sizeof of instantiated template?
(Removed private template member).
If yes, than
int_type __dummy;
is our all.
</snip>
I.e. problem can be easy resolved---i.e. ABI will not suffer, if we will
reach some consensus on the main issue.
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"
2017-11-16 11:39 ` Petr Ovtchenkov
@ 2017-11-16 11:41 ` Jonathan Wakely
2017-11-16 11:42 ` Jonathan Wakely
2017-11-16 12:03 ` Petr Ovtchenkov
0 siblings, 2 replies; 55+ messages in thread
From: Jonathan Wakely @ 2017-11-16 11:41 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
On 16/11/17 14:35 +0300, Petr Ovtchenkov wrote:
>On Thu, 16 Nov 2017 10:56:29 +0000
>Jonathan Wakely <jwakely@redhat.com> wrote:
>
>> On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
>> >This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
>> >(svn r253417).
>>
>> I'm not even going to bother to review patches sent without any
>> explanation or rationale for the change.
>
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00044.html
>
>Along with "violate principles of C++ objects life cycle",
>the side-effect is
>
> - Make istreambuf_iterator::_M_sbuf immutable
> - streambuf_iterator: avoid debug-dependent behaviour
>
>I should underline, that "_M_sbuf = 0" when istreambuf_iterator
>see eof, lead to cripple lifecycle of istreambuf_iterator
>object and [almost] block usage of istreambuf_iterator
>for entities other then immutable files.
>
>All tests from 24_iterators and 25_algorithms passed,
>so I expect it conform to Standard.
>
>This is series of patches, not single patch because
>I keep in mind technology aspect---easy transfer
>to branches other then trunk.
>
>>
>> I will repeat what Paolo said: changing the ABI is not acceptable.
>
>I will repeat special for you:
>
><snip>
>Is we really worry about frozen sizeof of instantiated template?
>(Removed private template member).
>
>If yes, than
>
> int_type __dummy;
>
>is our all.
></snip>
>
>I.e. problem can be easy resolved---i.e. ABI will not suffer, if we will
What about other translation units which have inlined the old
definition of the template, and expect to find a buffered character in
that member?
>reach some consensus on the main issue.
We don't have any consensus, in fact I don't see anybody agreeing with
you, and I've previously stated I don't want to support your use case:
https://gcc.gnu.org/ml/libstdc++/2017-09/msg00100.html
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"
2017-11-16 11:41 ` Jonathan Wakely
@ 2017-11-16 11:42 ` Jonathan Wakely
2017-11-16 12:03 ` Petr Ovtchenkov
1 sibling, 0 replies; 55+ messages in thread
From: Jonathan Wakely @ 2017-11-16 11:42 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
On 16/11/17 11:39 +0000, Jonathan Wakely wrote:
>On 16/11/17 14:35 +0300, Petr Ovtchenkov wrote:
>>On Thu, 16 Nov 2017 10:56:29 +0000
>>Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>>>On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
>>>>This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
>>>>(svn r253417).
>>>
>>>I'm not even going to bother to review patches sent without any
>>>explanation or rationale for the change.
>>
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00044.html
>>
>>Along with "violate principles of C++ objects life cycle",
>>the side-effect is
>>
>> - Make istreambuf_iterator::_M_sbuf immutable
>> - streambuf_iterator: avoid debug-dependent behaviour
>>
>>I should underline, that "_M_sbuf = 0" when istreambuf_iterator
>>see eof, lead to cripple lifecycle of istreambuf_iterator
>>object and [almost] block usage of istreambuf_iterator
>>for entities other then immutable files.
>>
>>All tests from 24_iterators and 25_algorithms passed,
>>so I expect it conform to Standard.
>>
>>This is series of patches, not single patch because
>>I keep in mind technology aspect---easy transfer
>>to branches other then trunk.
>>
>>>
>>>I will repeat what Paolo said: changing the ABI is not acceptable.
>>
>>I will repeat special for you:
>>
>><snip>
>>Is we really worry about frozen sizeof of instantiated template?
>>(Removed private template member).
>>
>>If yes, than
>>
>> int_type __dummy;
>>
>>is our all.
>></snip>
>>
>>I.e. problem can be easy resolved---i.e. ABI will not suffer, if we will
>
>What about other translation units which have inlined the old
>definition of the template, and expect to find a buffered character in
>that member?
In other words, the ABI is not just the "frozen sizeof".
>>reach some consensus on the main issue.
>
>We don't have any consensus, in fact I don't see anybody agreeing with
>you, and I've previously stated I don't want to support your use case:
>https://gcc.gnu.org/ml/libstdc++/2017-09/msg00100.html
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator
2017-11-16 11:35 ` Paolo Carlini
@ 2017-11-16 11:44 ` Petr Ovtchenkov
2017-11-16 11:57 ` Paolo Carlini
0 siblings, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-16 11:44 UTC (permalink / raw)
To: Paolo Carlini; +Cc: libstdc++, gcc-patches
On Thu, 16 Nov 2017 12:29:37 +0100
Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 16/11/2017 12:03, Petr Ovtchenkov wrote:
> > On Thu, 16 Nov 2017 10:39:02 +0100
> > Paolo Carlini <paolo.carlini@oracle.com> wrote:
> >
> >> Hi,
> >>
> >> On 16/11/2017 06:31, Petr Ovtchenkov wrote:
> >>> Is we really worry about frozen sizeof of instantiated template?
> >> Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
> >> under "Prohibited Changes", point 8.
> >>
> >> Of course removing the buffering has performance implications too -
> >> that's why it's there in the first place!
> > "buffering" here is a secondary buffering (after streambuf).
> > No relation to performance, but place for incoherence with
> > state of attached streambuf.
> It depends, we may be dealing with an unbuffered stream. For sure at the
> time we measured a performance impact in some cases, likewise whoever
> implemented it in the first place (not me) otherwise, again, why bothering?
This part of code is from SGI, so I suspect that nobody here really
measure performance difference between "bufferred" and "non-buffered"
implementations. Just because we have only implementation
with _M_c in isreambuf_iterator.
>
> Paolo.
>
--
- ptr
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks
2017-11-16 11:29 ` Jonathan Wakely
@ 2017-11-16 11:57 ` Jonathan Wakely
2017-11-16 12:25 ` Petr Ovtchenkov
2017-11-16 17:48 ` François Dumont
0 siblings, 2 replies; 55+ messages in thread
From: Jonathan Wakely @ 2017-11-16 11:57 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: François Dumont, libstdc++, gcc-patches
On 16/11/17 10:57 +0000, Jonathan Wakely wrote:
>On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
>>On Mon, 6 Nov 2017 22:19:22 +0100
>>François Dumont <frs.dumont@gmail.com> wrote:
>>
>>>Hi
>>>
>>> Â Â Â Any final decision regarding this patch ?
>>>
>>>François
>>
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
>
>It would be helpful if you two could collaborate and come up with a
>good solution, or at least discuss the pros and cons, instead of just
>sending competing patches.
Let me be more clear: I'm not going to review further patches in this
area while you two are proposing different alternatives, without
commenting on each other's approach.
If you think your solution is better than François's solution, you
should explain why, not just send a different patch. If François
thinks his solution is better than yours, he should state why, not
just send a different patch.
I don't have time to infer all that from just your patches, so I'm not
going to bother.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator
2017-11-16 11:44 ` Petr Ovtchenkov
@ 2017-11-16 11:57 ` Paolo Carlini
0 siblings, 0 replies; 55+ messages in thread
From: Paolo Carlini @ 2017-11-16 11:57 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
Hi,
On 16/11/2017 12:41, Petr Ovtchenkov wrote:
> This part of code is from SGI, so I suspect that nobody here really
> measure performance difference between "bufferred" and "non-buffered"
> implementations.
Here where? The GNU libstdc++-v3 implementation? Certainly we did, as I
tried to tell you the issue - the tradeoff between a more "correct",
simpler, and certainly easier to synchronize implementation and better
performance in some cases - isn't new. Please carry out a through search
of Bugzilla and mailing list, there must be something recorded. I'll see
if I can help about that, it's been a while.
Paolo.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"
2017-11-16 11:41 ` Jonathan Wakely
2017-11-16 11:42 ` Jonathan Wakely
@ 2017-11-16 12:03 ` Petr Ovtchenkov
1 sibling, 0 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-16 12:03 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
On Thu, 16 Nov 2017 11:39:30 +0000
Jonathan Wakely <jwakely@redhat.com> wrote:
> On 16/11/17 14:35 +0300, Petr Ovtchenkov wrote:
> >On Thu, 16 Nov 2017 10:56:29 +0000
> >Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> >> On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
> >> >This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
> >> >(svn r253417).
> >>
> >> I'm not even going to bother to review patches sent without any
> >> explanation or rationale for the change.
> >
> >https://gcc.gnu.org/ml/libstdc++/2017-11/msg00044.html
> >
> >Along with "violate principles of C++ objects life cycle",
> >the side-effect is
> >
> > - Make istreambuf_iterator::_M_sbuf immutable
> > - streambuf_iterator: avoid debug-dependent behaviour
> >
> >I should underline, that "_M_sbuf = 0" when istreambuf_iterator
> >see eof, lead to cripple lifecycle of istreambuf_iterator
> >object and [almost] block usage of istreambuf_iterator
> >for entities other then immutable files.
> >
> >All tests from 24_iterators and 25_algorithms passed,
> >so I expect it conform to Standard.
> >
> >This is series of patches, not single patch because
> >I keep in mind technology aspect---easy transfer
> >to branches other then trunk.
> >
> >>
> >> I will repeat what Paolo said: changing the ABI is not acceptable.
> >
> >I will repeat special for you:
> >
> ><snip>
> >Is we really worry about frozen sizeof of instantiated template?
> >(Removed private template member).
> >
> >If yes, than
> >
> > int_type __dummy;
> >
> >is our all.
> ></snip>
> >
> >I.e. problem can be easy resolved---i.e. ABI will not suffer, if we will
>
> What about other translation units which have inlined the old
> definition of the template, and expect to find a buffered character in
> that member?
I can say that I can write
int_type _M_c;
but you see, this is a _private_ member of template, so we should (may?) worry only
about size of object.
Just for clarification: Do you made accent on "buffered" or on "character" ("symbol" in ELF)?
>
> >reach some consensus on the main issue.
>
> We don't have any consensus, in fact I don't see anybody agreeing with
> you, and I've previously stated I don't want to support your use case:
> https://gcc.gnu.org/ml/libstdc++/2017-09/msg00100.html
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks
2017-11-16 11:57 ` Jonathan Wakely
@ 2017-11-16 12:25 ` Petr Ovtchenkov
2017-11-16 17:48 ` François Dumont
1 sibling, 0 replies; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-16 12:25 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: François Dumont, libstdc++, gcc-patches
On Thu, 16 Nov 2017 11:46:48 +0000
Jonathan Wakely <jwakely@redhat.com> wrote:
> On 16/11/17 10:57 +0000, Jonathan Wakely wrote:
> >On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
> >>On Mon, 6 Nov 2017 22:19:22 +0100
> >>François Dumont <frs.dumont@gmail.com> wrote:
> >>
> >>>Hi
> >>>
> >>> Any final decision regarding this patch ?
> >>>
> >>>François
> >>
> >>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
> >>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
> >>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
> >>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
> >
> >It would be helpful if you two could collaborate and come up with a
> >good solution, or at least discuss the pros and cons, instead of just
> >sending competing patches.
>
>
> Let me be more clear: I'm not going to review further patches in this
> area while you two are proposing different alternatives, without
> commenting on each other's approach.
>
> If you think your solution is better than François's solution, you
> should explain why, not just send a different patch. If François
> thinks his solution is better than yours, he should state why, not
> just send a different patch.
>
> I don't have time to infer all that from just your patches, so I'm not
> going to bother.
>
References here is a notification that
- there is another opinion;
- discussion is in another thread.
Nothing more.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks
2017-11-16 11:57 ` Jonathan Wakely
2017-11-16 12:25 ` Petr Ovtchenkov
@ 2017-11-16 17:48 ` François Dumont
2017-11-16 18:24 ` Petr Ovtchenkov
1 sibling, 1 reply; 55+ messages in thread
From: François Dumont @ 2017-11-16 17:48 UTC (permalink / raw)
To: Jonathan Wakely, Petr Ovtchenkov; +Cc: libstdc++, gcc-patches
On 16/11/2017 12:46, Jonathan Wakely wrote:
> On 16/11/17 10:57 +0000, Jonathan Wakely wrote:
>> On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
>>> On Mon, 6 Nov 2017 22:19:22 +0100
>>> François Dumont <frs.dumont@gmail.com> wrote:
>>>
>>>> Hi
>>>>
>>>> Â Â Â Any final decision regarding this patch ?
>>>>
>>>> François
>>>
>>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
>>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
>>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
>>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
>>
>> It would be helpful if you two could collaborate and come up with a
>> good solution, or at least discuss the pros and cons, instead of just
>> sending competing patches.
>
>
> Let me be more clear: I'm not going to review further patches in this
> area while you two are proposing different alternatives, without
> commenting on each other's approach.
>
> If you think your solution is better than François's solution, you
> should explain why, not just send a different patch. If François
> thinks his solution is better than yours, he should state why, not
> just send a different patch.
>
> I don't have time to infer all that from just your patches, so I'm not
> going to bother.
>
>
Proposing to revert my patch doesn't sound to me like a friendly action
to start a collaboration.
My only concern has always been the Debug mode impact which is now fixed.
I already said that I disagree with Petr's main goal to keep eof
iterator linked to the underlying stream. So current implementation is
just fine to me and I'll let Petr argument for any change. @Jonathan,
You can ignore my last request to remove mutable keywork on _M_sbuf.
François
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks
2017-11-16 17:48 ` François Dumont
@ 2017-11-16 18:24 ` Petr Ovtchenkov
2017-11-16 23:00 ` François Dumont
0 siblings, 1 reply; 55+ messages in thread
From: Petr Ovtchenkov @ 2017-11-16 18:24 UTC (permalink / raw)
To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On Thu, 16 Nov 2017 18:40:08 +0100
François Dumont <frs.dumont@gmail.com> wrote:
> On 16/11/2017 12:46, Jonathan Wakely wrote:
> > On 16/11/17 10:57 +0000, Jonathan Wakely wrote:
> >> On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
> >>> On Mon, 6 Nov 2017 22:19:22 +0100
> >>> François Dumont <frs.dumont@gmail.com> wrote:
> >>>
> >>>> Hi
> >>>>
> >>>> Any final decision regarding this patch ?
> >>>>
> >>>> François
> >>>
> >>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
> >>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
> >>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
> >>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
> >>
> >> It would be helpful if you two could collaborate and come up with a
> >> good solution, or at least discuss the pros and cons, instead of just
> >> sending competing patches.
> >
> >
> > Let me be more clear: I'm not going to review further patches in this
> > area while you two are proposing different alternatives, without
> > commenting on each other's approach.
> >
> > If you think your solution is better than François's solution, you
> > should explain why, not just send a different patch. If François
> > thinks his solution is better than yours, he should state why, not
> > just send a different patch.
> >
> > I don't have time to infer all that from just your patches, so I'm not
> > going to bother.
> >
> >
> Proposing to revert my patch doesn't sound to me like a friendly action
> to start a collaboration.
I'm already say that this is technical issue: this patch present only in
trunk yet. Series is more useful for applying in different branches.
BTW, https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
was inspired by you https://gcc.gnu.org/ml/libstdc++/2017-10/msg00029.html
>
> My only concern has always been the Debug mode impact which is now fixed.
I hope I'm suggest identical behaviour for Debug and non-Debug mode
(no difference in interaction with associated streambuf).
>
> I already said that I disagree with Petr's main goal to keep eof
> iterator linked to the underlying stream.
Ok.
> So current implementation is
> just fine to me and I'll let Petr argument for any change.
Please, clear for me: what is the "current implementation"?
Is it what we see now in trunk?
> @Jonathan,
> You can ignore my last request to remove mutable keywork on _M_sbuf.
>
> François
>
>
--
- ptr
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks
2017-11-16 18:24 ` Petr Ovtchenkov
@ 2017-11-16 23:00 ` François Dumont
0 siblings, 0 replies; 55+ messages in thread
From: François Dumont @ 2017-11-16 23:00 UTC (permalink / raw)
To: Petr Ovtchenkov; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On 16/11/2017 19:12, Petr Ovtchenkov wrote:
> On Thu, 16 Nov 2017 18:40:08 +0100
> François Dumont <frs.dumont@gmail.com> wrote:
>
>> On 16/11/2017 12:46, Jonathan Wakely wrote:
>>
>>> Let me be more clear: I'm not going to review further patches in this
>>> area while you two are proposing different alternatives, without
>>> commenting on each other's approach.
>>>
>>> If you think your solution is better than François's solution, you
>>> should explain why, not just send a different patch. If François
>>> thinks his solution is better than yours, he should state why, not
>>> just send a different patch.
>>>
>>> I don't have time to infer all that from just your patches, so I'm not
>>> going to bother.
>>>
>>>
>> Proposing to revert my patch doesn't sound to me like a friendly action
>> to start a collaboration.
> I'm already say that this is technical issue: this patch present only in
> trunk yet.
Doesn't explain why you want to revert it.
> Series is more useful for applying in different branches.
Which branches ? There are mostly maintenance branches. None of your
patches is fixing a bug so it won't go to a maintenance branch.
> BTW, https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
> was inspired by you https://gcc.gnu.org/ml/libstdc++/2017-10/msg00029.html
Which was already rejected, why submitting it again ?
>
>> So current implementation is
>> just fine to me and I'll let Petr argument for any change.
> Please, clear for me: what is the "current implementation"?
> Is it what we see now in trunk?
Yes. Other proposals were mostly code quality changes. None of them had
no impact so Jonathan decision to keep current implementation is just fine.
^ permalink raw reply [flat|nested] 55+ messages in thread
end of thread, other threads:[~2017-11-16 21:31 UTC | newest]
Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-23 7:10 [PATCH] libstdc++: istreambuf_iterator keep attached streambuf Petr Ovtchenkov
2017-09-25 13:46 ` Jonathan Wakely
2017-09-28 10:34 ` Jonathan Wakely
2017-09-28 12:06 ` Petr Ovtchenkov
2017-09-28 12:38 ` Jonathan Wakely
2017-10-03 20:39 ` Petr Ovtchenkov
2017-10-04 5:04 ` [PATCH v2] " Petr Ovtchenkov
2017-10-06 16:01 ` [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf) François Dumont
2017-10-06 18:03 ` Petr Ovtchenkov
2017-10-08 15:45 ` [PATCH] libstdc++: istreambuf_iterator proxy François Dumont
2017-10-09 19:35 ` Petr Ovtchenkov
2017-10-10 5:52 ` Petr Ovtchenkov
2017-10-10 14:22 ` [PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf) Jonathan Wakely
-- strict thread matches above, loose matches on Subject: below --
2017-10-13 17:22 Make istreambuf_iterator::_M_sbuf immutable and add debug checks François Dumont
2017-10-23 19:24 ` François Dumont
2017-11-06 21:20 ` François Dumont
2017-11-16 6:07 ` Petr Ovtchenkov
2017-11-16 11:29 ` Jonathan Wakely
2017-11-16 11:57 ` Jonathan Wakely
2017-11-16 12:25 ` Petr Ovtchenkov
2017-11-16 17:48 ` François Dumont
2017-11-16 18:24 ` Petr Ovtchenkov
2017-11-16 23:00 ` François Dumont
2017-09-27 20:16 Make tests less istreambuf_iterator implementation dependent François Dumont
2017-09-28 12:12 ` Jonathan Wakely
2017-09-28 19:59 ` François Dumont
2017-09-28 21:56 ` Jonathan Wakely
2017-10-02 5:43 ` François Dumont
2017-10-03 14:20 ` Jonathan Wakely
2017-10-04 16:21 ` François Dumont
2017-10-04 23:23 ` Jonathan Wakely
2017-11-15 20:54 ` [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" Petr Ovtchenkov
2017-11-15 20:58 ` [PATCH 2/4] libstdc++: istreambuf_iterator keep attached streambuf Petr Ovtchenkov
2017-11-15 20:52 ` [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator Petr Ovtchenkov
2017-11-15 20:52 ` [PATCH 4/4] libstdc++: immutable _M_sbuf " Petr Ovtchenkov
2017-11-15 22:30 ` [PATCH 3/4] libstdc++: avoid character accumulation " Paolo Carlini
2017-11-16 6:00 ` Petr Ovtchenkov
2017-11-16 9:51 ` Paolo Carlini
2017-11-16 11:34 ` Petr Ovtchenkov
2017-11-16 11:35 ` Paolo Carlini
2017-11-16 11:44 ` Petr Ovtchenkov
2017-11-16 11:57 ` Paolo Carlini
2017-11-16 11:03 ` [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" Jonathan Wakely
2017-11-16 11:39 ` Petr Ovtchenkov
2017-11-16 11:41 ` Jonathan Wakely
2017-11-16 11:42 ` Jonathan Wakely
2017-11-16 12:03 ` Petr Ovtchenkov
2017-08-24 11:58 [PATCH] streambuf_iterator: avoid debug-dependent behaviour Petr Ovtchenkov
2017-09-01 9:10 ` Jonathan Wakely
2017-09-07 21:02 ` François Dumont
2017-09-08 5:47 ` Petr Ovtchenkov
2017-09-09 20:17 ` François Dumont
2017-09-21 5:46 ` François Dumont
2017-09-28 10:50 ` Jonathan Wakely
2017-09-28 10:58 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).