| Message ID | cd085d750d6518747a71b80a55128e4639564746.1510778853.git.ptr@void-ptr.info |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>" | expand |
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.
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. >
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
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 >
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 >
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; }