This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] streambuf_iterator: avoid debug-dependent behaviour
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
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 );