New istreambuf_iterator debug check

Message ID e54204f1-ece7-319d-095e-a5380f5a262c@gmail.com
State New
Headers show
Series
  • New istreambuf_iterator debug check
Related show

Commit Message

François Dumont Jan. 24, 2018, 4:39 p.m.
Hi

     I'd like to propose this new debug check. Comparing with non-eos 
istreambuf_iterator sounds like an obvious coding mistake.

     I propose it despite the stage 1 as it is just a new debug check, 
it doesn't impact the lib in normal mode.

     Tested under Linux x86_64, ok to commit ?

François

Comments

Petr Ovtchenkov Jan. 24, 2018, 5:53 p.m. | #1
On Wed, 24 Jan 2018 17:39:59 +0100
François Dumont <frs.dumont@gmail.com> wrote:

> Hi

> 

>      I'd like to propose this new debug check. Comparing with non-eos 

> istreambuf_iterator sounds like an obvious coding mistake.

> 

>      I propose it despite the stage 1 as it is just a new debug check, 

> it doesn't impact the lib in normal mode.

> 

>      Tested under Linux x86_64, ok to commit ?

> 

> François

> 


       bool
       equal(const istreambuf_iterator& __b) const
-      { return _M_at_eof() == __b._M_at_eof(); }
+      {
+	bool __this_at_eof = _M_at_eof();
+	bool __b_at_eof = __b._M_at_eof();
+
+	__glibcxx_requires_cond(__this_at_eof || __b_at_eof, _M_message(
+	  "Abnormal comparison to non-end-of-stream istreambuf_iterator"));
+	return __this_at_eof == __b_at_eof;
+      }

Looks strange for me. It is legal and possible that istreambuf_iterator
will be in EOF state.

--

  - ptr
François Dumont Jan. 24, 2018, 8:34 p.m. | #2
On 24/01/2018 18:53, Petr Ovtchenkov wrote:
> On Wed, 24 Jan 2018 17:39:59 +0100

> François Dumont <frs.dumont@gmail.com> wrote:

>

>> Hi

>>

>>       I'd like to propose this new debug check. Comparing with non-eos

>> istreambuf_iterator sounds like an obvious coding mistake.

>>

>>       I propose it despite the stage 1 as it is just a new debug check,

>> it doesn't impact the lib in normal mode.

>>

>>       Tested under Linux x86_64, ok to commit ?

>>

>> François

>>

>         bool

>         equal(const istreambuf_iterator& __b) const

> -      { return _M_at_eof() == __b._M_at_eof(); }

> +      {

> +	bool __this_at_eof = _M_at_eof();

> +	bool __b_at_eof = __b._M_at_eof();

> +

> +	__glibcxx_requires_cond(__this_at_eof || __b_at_eof, _M_message(

> +	  "Abnormal comparison to non-end-of-stream istreambuf_iterator"));

> +	return __this_at_eof == __b_at_eof;

> +      }

>

> Looks strange for me. It is legal and possible that istreambuf_iterator

> will be in EOF state.

>

Sure, but consider rather the associated 3_neg.cc showing the debug 
check purpose:

   cistreambuf_iter it1(istrs), it2(istrs);
   it1 == it2; // No sens
Petr Ovtchenkov Jan. 25, 2018, 5:38 a.m. | #3
On Wed, 24 Jan 2018 21:34:48 +0100
François Dumont <frs.dumont@gmail.com> wrote:

> On 24/01/2018 18:53, Petr Ovtchenkov wrote:

> > On Wed, 24 Jan 2018 17:39:59 +0100

> > François Dumont <frs.dumont@gmail.com> wrote:

> >

> >> Hi

> >>

> >>       I'd like to propose this new debug check. Comparing with non-eos

> >> istreambuf_iterator sounds like an obvious coding mistake.

> >>

> >>       I propose it despite the stage 1 as it is just a new debug check,

> >> it doesn't impact the lib in normal mode.

> >>

> >>       Tested under Linux x86_64, ok to commit ?

> >>

> >> François

> >>

> >         bool

> >         equal(const istreambuf_iterator& __b) const

> > -      { return _M_at_eof() == __b._M_at_eof(); }

> > +      {

> > +	bool __this_at_eof = _M_at_eof();

> > +	bool __b_at_eof = __b._M_at_eof();

> > +

> > +	__glibcxx_requires_cond(__this_at_eof || __b_at_eof, _M_message(

> > +	  "Abnormal comparison to non-end-of-stream istreambuf_iterator"));

> > +	return __this_at_eof == __b_at_eof;

> > +      }

> >

> > Looks strange for me. It is legal and possible that istreambuf_iterator

> > will be in EOF state.

> >

> Sure, but consider rather the associated 3_neg.cc showing the debug 

> check purpose:

> 

>    cistreambuf_iter it1(istrs), it2(istrs);

>    it1 == it2; // No sens

> 


This is what author want to say.

Neveretheless, __glibcxx_requires_cond(__this_at_eof || __b_at_eof, ...
in equal looks bogus for me.

--

  - ptr
Jonathan Wakely Jan. 29, 2018, 1:52 p.m. | #4
On 24/01/18 17:39 +0100, François Dumont wrote:
>Hi

>

>    I'd like to propose this new debug check. Comparing with non-eos 

>istreambuf_iterator sounds like an obvious coding mistake.


Agreed, but that doesn't mean we can terminate the process. It's still
valid C++, even though it's probably not what the author intended to do.

Patch

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 292ef3a..2e46771 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -174,7 +174,14 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       /// Return true both iterators are end or both are not end.
       bool
       equal(const istreambuf_iterator& __b) const
-      { return _M_at_eof() == __b._M_at_eof(); }
+      {
+	bool __this_at_eof = _M_at_eof();
+	bool __b_at_eof = __b._M_at_eof();
+
+	__glibcxx_requires_cond(__this_at_eof || __b_at_eof, _M_message(
+	  "Abnormal comparison to non-end-of-stream istreambuf_iterator"));
+	return __this_at_eof == __b_at_eof;
+      }
 
     private:
       int_type
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/1.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/1.cc
new file mode 100644
index 0000000..bb2e6f8
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/1.cc
@@ -0,0 +1,50 @@ 
+// Copyright (C) 2018 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 <sstream>
+#include <iterator>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  std::istringstream istrs("123456789");
+  typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+  cistreambuf_iter it(istrs);
+  cistreambuf_iter eof;
+  VERIFY( !it.equal(eof) );
+  VERIFY( !eof.equal(it) );
+  VERIFY( it != eof );
+  VERIFY( eof != it );
+}
+
+void test02()
+{
+  typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+  cistreambuf_iter eof;
+  VERIFY( eof.equal(eof) );
+  VERIFY( eof == eof );
+}
+
+int main()
+{
+  test01();
+  test02();
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
index 3fe1cf1..47a1a00 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -47,7 +47,10 @@  void test02(void)
 
   cistreambuf_iter istrb_it05(istrs01);
   cistreambuf_iter istrb_it06(istrs01.rdbuf());
+
+#ifndef _GLIBCXX_DEBUG
   VERIFY( istrb_it05 == istrb_it06 );
+#endif
   
   // bool equal(istreambuf_iter& b)
   cistreambuf_iter istrb_it07(0);
@@ -57,12 +60,14 @@  void test02(void)
   cistreambuf_iter istrb_it10;
   VERIFY( istrb_it10.equal(istrb_it09) );
 
+#ifndef _GLIBCXX_DEBUG
   cistreambuf_iter istrb_it11(istrs01);
   cistreambuf_iter istrb_it12(istrs01.rdbuf());
   VERIFY( istrb_it11.equal(istrb_it12) );
   cistreambuf_iter istrb_it13(istrs01);
   cistreambuf_iter istrb_it14(istrs01.rdbuf());
   VERIFY( istrb_it14.equal(istrb_it13) );
+#endif
 
   cistreambuf_iter istrb_it15(istrs01);
   cistreambuf_iter istrb_it16;
@@ -77,9 +82,11 @@  void test02(void)
   cistreambuf_iter istrb_it20;
   VERIFY( istrb_it19 == istrb_it20 );
 
+#ifndef _GLIBCXX_DEBUG
   cistreambuf_iter istrb_it21(istrs01);
   cistreambuf_iter istrb_it22(istrs01.rdbuf());
   VERIFY( istrb_it22 == istrb_it21 );
+#endif
 
   cistreambuf_iter istrb_it23(istrs01);
   cistreambuf_iter istrb_it24;
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/3_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/3_neg.cc
new file mode 100644
index 0000000..5e75704
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/3_neg.cc
@@ -0,0 +1,37 @@ 
+// Copyright (C) 2018 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 <sstream>
+#include <iterator>
+
+void test01()
+{
+  std::istringstream istrs("123456789");
+  typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+  cistreambuf_iter it1(istrs), it2(istrs);
+  it1 == it2; // No sens
+}
+
+int main()
+{
+  test01();
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/25_algorithms/find/istreambuf_iterators/char/1.cc b/libstdc++-v3/testsuite/25_algorithms/find/istreambuf_iterators/char/1.cc
index 0b2a8fb..fa5d314 100644
--- a/libstdc++-v3/testsuite/25_algorithms/find/istreambuf_iterators/char/1.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/find/istreambuf_iterators/char/1.cc
@@ -36,7 +36,7 @@  void test01()
   in_iterator_type end1, it1;
 
   it1 = find(beg1, beg1, 'l');
-  VERIFY( it1 == beg1 );
+  VERIFY( it1 != end1 );
   VERIFY( *it1 == 'D' );
 
   it1 = find(end1, end1, 'D');
diff --git a/libstdc++-v3/testsuite/25_algorithms/find/istreambuf_iterators/wchar_t/1.cc b/libstdc++-v3/testsuite/25_algorithms/find/istreambuf_iterators/wchar_t/1.cc
index bfb476b..84766fb 100644
--- a/libstdc++-v3/testsuite/25_algorithms/find/istreambuf_iterators/wchar_t/1.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/find/istreambuf_iterators/wchar_t/1.cc
@@ -36,7 +36,7 @@  void test01()
   in_iterator_type end1, it1;
 
   it1 = find(beg1, beg1, L'l');
-  VERIFY( it1 == beg1 );
+  VERIFY( it1 != end1 );
   VERIFY( *it1 == L'D' );
 
   it1 = find(end1, end1, L'D');