Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[François Dumont]

On 16/11/2017 19:12, Petr Ovtchenkov wrote:

On Thu, 16 Nov 2017 18:40:08 +0100
François Dumont  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.

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 18:40:08 +0100
François Dumont  wrote:

> On 16/11/2017 12:46, Jonathan Wakely wrote:
> > On 16/11/17 10:57 +, 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  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

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[François Dumont]

On 16/11/2017 12:46, Jonathan Wakely wrote:

On 16/11/17 10:57 +, 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  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

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 11:46:48 +
Jonathan Wakely  wrote:

> On 16/11/17 10:57 +, 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  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.

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Jonathan Wakely]

On 16/11/17 10:57 +, 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  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.

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Jonathan Wakely]

On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:

On Mon, 6 Nov 2017 22:19:22 +0100
François Dumont  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.

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Petr Ovtchenkov]

On Mon, 6 Nov 2017 22:19:22 +0100
François Dumont  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

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[François Dumont]

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

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[François Dumont]

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






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(liffey.getloc());
 
   typedef