public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* Add std::copy_n overload for istreambuf_iterator
@ 2019-07-19 21:38 François Dumont
  2019-09-27 11:01 ` Jonathan Wakely
  0 siblings, 1 reply; 8+ messages in thread
From: François Dumont @ 2019-07-19 21:38 UTC (permalink / raw)
  To: libstdc++, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

It sounds reasonable to overload std::copy_n for istreambuf_iterator.

     * include/bits/stl_algo.h (copy_n(istreambuf_iterator<>, _Size, 
_OIte)):
     New declaration.
     * include/bits/streambuf_iterator.h (istreambuf_iterator<>): Declare
     std::copy_n for istreambuf_iterator of char types to be friend.
     (std::copy_n(istreambuf_iterator<>, _Size, _OIte)): New overload.
     * include/std/streambuf(basic_streambuf<>): Declare std::copy_n for
     istreambuf_iterator of char types to be friend.
     * testsuite/25_algorithms/copy_n/istreambuf_iterator.cc: New.
     * testsuite/25_algorithms/copy_n/istreambuf_iterator_neg.cc: New.

Tested under Linux x86_64, normal and debug modes.

Ok to commit ?

François


[-- Attachment #2: istreambuf_copy_n.patch --]
[-- Type: text/x-patch, Size: 7290 bytes --]

diff --git a/libstdc++-v3/include/bits/stl_algo.h b/libstdc++-v3/include/bits/stl_algo.h
index 478f012def8..ec651e2cc45 100644
--- a/libstdc++-v3/include/bits/stl_algo.h
+++ b/libstdc++-v3/include/bits/stl_algo.h
@@ -771,6 +771,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	     _OutputIterator __result, random_access_iterator_tag)
     { return std::copy(__first, __first + __n, __result); }
 
+  template<typename _CharT, typename _Size, typename _OutputIterator>
+    typename enable_if<__is_char<_CharT>::__value,
+		       _OutputIterator>::type
+    copy_n(istreambuf_iterator<_CharT, char_traits<_CharT> >,
+	   _Size __n, _OutputIterator __result);
+
   /**
    *  @brief Copies the range [first,first+n) into [result,result+n).
    *  @ingroup mutating_algorithms
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 2f4ff494a3a..c682fa91bde 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -80,6 +80,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	__copy_move_a2(istreambuf_iterator<_CharT2>,
 		       istreambuf_iterator<_CharT2>, _CharT2*);
 
+#if __cplusplus >= 201103L
+      template<typename _CharT2, typename _Size, typename _OutputIterator>
+	friend typename enable_if<__is_char<_CharT2>::__value,
+				  _OutputIterator>::type
+	copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
+#endif
+
       template<typename _CharT2>
 	friend typename __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
 				    istreambuf_iterator<_CharT2> >::__type
@@ -367,6 +374,50 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return __result;
     }
 
+#if __cplusplus >= 201103L
+  template<typename _CharT, typename _Size, typename _OutputIterator>
+    typename enable_if<__is_char<_CharT>::__value, _OutputIterator>::type
+    copy_n(istreambuf_iterator<_CharT> __it, _Size __n,
+	   _OutputIterator __result)
+    {
+      if (__n == 0)
+	return __result;
+
+      __glibcxx_assert(__n > 0);
+      __glibcxx_requires_cond(!__it._M_at_eof(),
+			      _M_message(__gnu_debug::__msg_inc_istreambuf)
+			      ._M_iterator(__it));
+
+      using traits_type = typename istreambuf_iterator<_CharT>::traits_type;
+      const auto __eof = traits_type::eof();
+
+      auto __sb = __it._M_sbuf;
+      while (__n > 0)
+	{
+	  streamsize __size = __sb->egptr() - __sb->gptr();
+	  if (__size == 0)
+	    {
+	      if (traits_type::eq_int_type(__sb->underflow(), __eof))
+		{
+		  __glibcxx_requires_cond(__n == 0,
+		    _M_message(__gnu_debug::__msg_inc_istreambuf)
+					  ._M_iterator(__it));
+		  break;
+		}
+
+	      __size =  __sb->egptr() - __sb->gptr();
+	    }
+
+	  streamsize __xsize = std::min<streamsize>(__size, __n);
+	  __result = std::copy(__sb->gptr(), __sb->gptr() + __xsize, __result);
+	  __sb->__safe_gbump(__xsize);
+	  __n -= __xsize;
+	}
+
+      return __result;
+    }
+#endif
+
   template<typename _CharT>
     typename __gnu_cxx::__enable_if<__is_char<_CharT>::__value,
 		  		    istreambuf_iterator<_CharT> >::__type
diff --git a/libstdc++-v3/include/std/streambuf b/libstdc++-v3/include/std/streambuf
index d9ca981d704..4f62ebf4d95 100644
--- a/libstdc++-v3/include/std/streambuf
+++ b/libstdc++-v3/include/std/streambuf
@@ -155,6 +155,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
         __copy_move_a2(istreambuf_iterator<_CharT2>,
 		       istreambuf_iterator<_CharT2>, _CharT2*);
 
+#if __cplusplus >= 201103L
+      template<typename _CharT2, typename _Size, typename _OutputIterator>
+	friend typename enable_if<__is_char<_CharT2>::__value,
+				  _OutputIterator>::type
+	copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
+#endif
+
       template<typename _CharT2>
         friend typename __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
 				  istreambuf_iterator<_CharT2> >::__type
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
new file mode 100644
index 00000000000..ebd769cf7c0
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
@@ -0,0 +1,59 @@
+// Copyright (C) 2019 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 { target c++11 } }
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  std::stringstream ss("12345");
+
+  std::string ostr(5, '0');
+  typedef std::istreambuf_iterator<char> istrb_ite;
+  std::copy_n(istrb_ite(ss), 0, ostr.begin());
+  VERIFY( ostr.front() == '0' );
+
+  std::copy_n(istrb_ite(ss), 2, ostr.begin());
+  VERIFY( ostr == "12000" );
+
+  std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2);
+  VERIFY( ostr == "12345" );
+}
+
+void test02()
+{
+  std::stringstream ss("12345");
+
+  std::string ostr(5, '0');
+  typedef std::istreambuf_iterator<char> istrb_ite;
+
+  istrb_ite ibfit(ss);
+  std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, ostr.begin()));
+  VERIFY( ostr == "12345" );
+}
+
+int main()
+{
+  test01();
+  test02();
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator_neg.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator_neg.cc
new file mode 100644
index 00000000000..42bc5b64306
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator_neg.cc
@@ -0,0 +1,40 @@
+// Copyright (C) 2019 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 { target c++11 xfail *-*-* } }
+// { dg-require-debug-mode { } }
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  std::stringstream ss("12345");
+
+  std::string ostr(10, '0');
+  typedef std::istreambuf_iterator<char> istrb_ite;
+  std::copy_n(istrb_ite(ss), 10, ostr.begin());
+}
+
+int main()
+{
+  test01();
+  return 0;
+}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Add std::copy_n overload for istreambuf_iterator
  2019-07-19 21:38 Add std::copy_n overload for istreambuf_iterator François Dumont
@ 2019-09-27 11:01 ` Jonathan Wakely
  2019-10-04  5:01   ` François Dumont
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Wakely @ 2019-09-27 11:01 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

On 19/07/19 23:37 +0200, François Dumont wrote:
>It sounds reasonable to overload std::copy_n for istreambuf_iterator.
>
>    * include/bits/stl_algo.h (copy_n(istreambuf_iterator<>, _Size, 
>_OIte)):
>    New declaration.
>    * include/bits/streambuf_iterator.h (istreambuf_iterator<>): Declare
>    std::copy_n for istreambuf_iterator of char types to be friend.
>    (std::copy_n(istreambuf_iterator<>, _Size, _OIte)): New overload.
>    * include/std/streambuf(basic_streambuf<>): Declare std::copy_n for
>    istreambuf_iterator of char types to be friend.
>    * testsuite/25_algorithms/copy_n/istreambuf_iterator.cc: New.
>    * testsuite/25_algorithms/copy_n/istreambuf_iterator_neg.cc: New.
>
>Tested under Linux x86_64, normal and debug modes.
>
>Ok to commit ?

This is a nice improvement, I just have a few minor comments on it...

>François
>

>diff --git a/libstdc++-v3/include/bits/stl_algo.h b/libstdc++-v3/include/bits/stl_algo.h
>index 478f012def8..ec651e2cc45 100644
>--- a/libstdc++-v3/include/bits/stl_algo.h
>+++ b/libstdc++-v3/include/bits/stl_algo.h
>@@ -771,6 +771,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	     _OutputIterator __result, random_access_iterator_tag)
>     { return std::copy(__first, __first + __n, __result); }
> 
>+  template<typename _CharT, typename _Size, typename _OutputIterator>
>+    typename enable_if<__is_char<_CharT>::__value,
>+		       _OutputIterator>::type

We have __enable_if_t that can be used to simplify this a bit:

    __enable_if_t<__is_char<_CharT>::__value, _OutputIterator>

(The other declarations in bits/streambuf_iterator.h would need to be
changed to use that as well).

>+    copy_n(istreambuf_iterator<_CharT, char_traits<_CharT> >,

std::copy_n is only declared for C++11 and later, so you don't need
the space between the closing angle brackets: >>

>+	   _Size __n, _OutputIterator __result);
>+
>   /**
>    *  @brief Copies the range [first,first+n) into [result,result+n).
>    *  @ingroup mutating_algorithms
>diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
>index 2f4ff494a3a..c682fa91bde 100644
>--- a/libstdc++-v3/include/bits/streambuf_iterator.h
>+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
>@@ -80,6 +80,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	__copy_move_a2(istreambuf_iterator<_CharT2>,
> 		       istreambuf_iterator<_CharT2>, _CharT2*);
> 
>+#if __cplusplus >= 201103L
>+      template<typename _CharT2, typename _Size, typename _OutputIterator>
>+	friend typename enable_if<__is_char<_CharT2>::__value,
>+				  _OutputIterator>::type
>+	copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>+#endif
>+
>       template<typename _CharT2>
> 	friend typename __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
> 				    istreambuf_iterator<_CharT2> >::__type
>@@ -367,6 +374,50 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       return __result;
>     }
> 
>+#if __cplusplus >= 201103L
>+  template<typename _CharT, typename _Size, typename _OutputIterator>
>+    typename enable_if<__is_char<_CharT>::__value, _OutputIterator>::type
>+    copy_n(istreambuf_iterator<_CharT> __it, _Size __n,
>+	   _OutputIterator __result)
>+    {
>+      if (__n == 0)
>+	return __result;
>+
>+      __glibcxx_assert(__n > 0);
>+      __glibcxx_requires_cond(!__it._M_at_eof(),
>+			      _M_message(__gnu_debug::__msg_inc_istreambuf)
>+			      ._M_iterator(__it));

Is this assertion necessary? Doesn't trying to read from the streambuf
also check the same condition, and so will already fail?

>+
>+      using traits_type = typename istreambuf_iterator<_CharT>::traits_type;

This is just char_traits<_CharT>, right?

>+      const auto __eof = traits_type::eof();
>+
>+      auto __sb = __it._M_sbuf;
>+      while (__n > 0)
>+	{
>+	  streamsize __size = __sb->egptr() - __sb->gptr();
>+	  if (__size == 0)
>+	    {
>+	      if (traits_type::eq_int_type(__sb->underflow(), __eof))
>+		{
>+		  __glibcxx_requires_cond(__n == 0,
>+		    _M_message(__gnu_debug::__msg_inc_istreambuf)
>+					  ._M_iterator(__it));
>+		  break;
>+		}
>+
>+	      __size =  __sb->egptr() - __sb->gptr();
>+	    }
>+
>+	  streamsize __xsize = std::min<streamsize>(__size, __n);
>+	  __result = std::copy(__sb->gptr(), __sb->gptr() + __xsize, __result);
>+	  __sb->__safe_gbump(__xsize);
>+	  __n -= __xsize;
>+	}
>+
>+      return __result;
>+    } 

I wonder whether it's worth doing:

#if __cplusplus >= 201703L
    if constexpr (is_same_v<_OutputIterator, _CharT*>)
      return __result + __it._M_sbuf->sgetn(__result, __n);
    else
      {
#endif
      ...
#if __cplusplus >= 201703L
      }
#endif

We could extend that to also work for basic_string<_CharT>::iterator
and vector<_CharT>::iterator too if we wanted.

I'm not sure if it will perform any better than the code below (it's
approximately equivalent) but it should result in smaller binaries, as we
wouldn't be instantiating the code below when outputting to a pointer
or contiguous iterator.

We don't need to do that now, it can be a separate patch later (if we
do it at all).

>+#endif

Because the matching #if is more than 40 lines away, please add a
comment noting the condition that this corresponds to, i.e.

#endif // C++11

>+
>   template<typename _CharT>
>     typename __gnu_cxx::__enable_if<__is_char<_CharT>::__value,
> 		  		    istreambuf_iterator<_CharT> >::__type
>diff --git a/libstdc++-v3/include/std/streambuf b/libstdc++-v3/include/std/streambuf
>index d9ca981d704..4f62ebf4d95 100644
>--- a/libstdc++-v3/include/std/streambuf
>+++ b/libstdc++-v3/include/std/streambuf
>@@ -155,6 +155,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>         __copy_move_a2(istreambuf_iterator<_CharT2>,
> 		       istreambuf_iterator<_CharT2>, _CharT2*);
> 
>+#if __cplusplus >= 201103L
>+      template<typename _CharT2, typename _Size, typename _OutputIterator>
>+	friend typename enable_if<__is_char<_CharT2>::__value,
>+				  _OutputIterator>::type
>+	copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>+#endif
>+
>       template<typename _CharT2>
>         friend typename __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
> 				  istreambuf_iterator<_CharT2> >::__type
>diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>new file mode 100644
>index 00000000000..ebd769cf7c0
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>@@ -0,0 +1,59 @@
>+// Copyright (C) 2019 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 { target c++11 } }
>+
>+#include <algorithm>
>+#include <sstream>
>+#include <iterator>
>+
>+#include <testsuite_hooks.h>
>+
>+void test01()
>+{
>+  std::stringstream ss("12345");
>+
>+  std::string ostr(5, '0');
>+  typedef std::istreambuf_iterator<char> istrb_ite;
>+  std::copy_n(istrb_ite(ss), 0, ostr.begin());
>+  VERIFY( ostr.front() == '0' );

I'd like to see a check here that the value returned from copy_n is
equal to ostr.begin().

>+
>+  std::copy_n(istrb_ite(ss), 2, ostr.begin());
>+  VERIFY( ostr == "12000" );

And equal to ostr.begin() + 2.

>+
>+  std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2);
>+  VERIFY( ostr == "12345" );

And equal to ostr.begin() + 5 here.

>+}
>+
>+void test02()
>+{
>+  std::stringstream ss("12345");
>+
>+  std::string ostr(5, '0');
>+  typedef std::istreambuf_iterator<char> istrb_ite;
>+
>+  istrb_ite ibfit(ss);
>+  std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, ostr.begin()));
>+  VERIFY( ostr == "12345" );
>+}
>

Ideally I'd also like to see tests where the input buffer is larger
than the size being read, e.g. read 5 chars from "123456" and verify
we don't read the '6'.

Also, these tests don't exercise the code path that causes an
underflow. It would be good to use an ifstream to read from one of the
files in the testsuite/data directory, and read a large amount of data
(more than fits in a filebuf's read area) so that the underflow logic
is tested.


>+int main()
>+{
>+  test01();
>+  test02();
>+  return 0;
>+}
>diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator_neg.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator_neg.cc
>new file mode 100644
>index 00000000000..42bc5b64306
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator_neg.cc
>@@ -0,0 +1,40 @@
>+// Copyright (C) 2019 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 { target c++11 xfail *-*-* } }
>+// { dg-require-debug-mode { } }
>+
>+#include <algorithm>
>+#include <sstream>
>+#include <iterator>
>+
>+#include <testsuite_hooks.h>
>+
>+void test01()
>+{
>+  std::stringstream ss("12345");
>+
>+  std::string ostr(10, '0');
>+  typedef std::istreambuf_iterator<char> istrb_ite;
>+  std::copy_n(istrb_ite(ss), 10, ostr.begin());
>+}
>+
>+int main()
>+{
>+  test01();
>+  return 0;
>+}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Add std::copy_n overload for istreambuf_iterator
  2019-09-27 11:01 ` Jonathan Wakely
@ 2019-10-04  5:01   ` François Dumont
  2019-10-04 11:06     ` Jonathan Wakely
  2019-10-09 20:18     ` Christophe Lyon
  0 siblings, 2 replies; 8+ messages in thread
From: François Dumont @ 2019-10-04  5:01 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 6645 bytes --]

On 9/27/19 1:00 PM, Jonathan Wakely wrote:
> On 19/07/19 23:37 +0200, François Dumont wrote:
>> It sounds reasonable to overload std::copy_n for istreambuf_iterator.
> I wonder whether it's worth doing:
>
> #if __cplusplus >= 201703L
>    if constexpr (is_same_v<_OutputIterator, _CharT*>)
>      return __result + __it._M_sbuf->sgetn(__result, __n);
>    else
>      {
> #endif
>      ...
> #if __cplusplus >= 201703L
>      }
> #endif
>
> We could extend that to also work for basic_string<_CharT>::iterator
> and vector<_CharT>::iterator too if we wanted.
>
> I'm not sure if it will perform any better than the code below (it's
> approximately equivalent) but it should result in smaller binaries, as we
> wouldn't be instantiating the code below when outputting to a pointer
> or contiguous iterator.
>
> We don't need to do that now, it can be a separate patch later (if we
> do it at all).

Very good remark, I hadn't check streambuf to find out if there were 
better to do. For me it is the streambuf method to target for an 
std::copy_n overload.

So here is a new proposal much simpler. I see no reason to enable it 
only for char types, is there ?

Once the other patch on copy/copy_backward... algos is in I'll provide 
what necessary to benefit from the same optimization for std::deque 
iterators and in Debug mode.

>
>> +#endif
>
> Because the matching #if is more than 40 lines away, please add a
> comment noting the condition that this corresponds to, i.e.
>
> #endif // C++11
Ok, done even if there is no 40 lines anymore. And also added it in 
stl_algo.h.
>
>> +
>>   template<typename _CharT>
>>     typename __gnu_cxx::__enable_if<__is_char<_CharT>::__value,
>>                       istreambuf_iterator<_CharT> >::__type
>> diff --git a/libstdc++-v3/include/std/streambuf 
>> b/libstdc++-v3/include/std/streambuf
>> index d9ca981d704..4f62ebf4d95 100644
>> --- a/libstdc++-v3/include/std/streambuf
>> +++ b/libstdc++-v3/include/std/streambuf
>> @@ -155,6 +155,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>         __copy_move_a2(istreambuf_iterator<_CharT2>,
>>                istreambuf_iterator<_CharT2>, _CharT2*);
>>
>> +#if __cplusplus >= 201103L
>> +      template<typename _CharT2, typename _Size, typename 
>> _OutputIterator>
>> +    friend typename enable_if<__is_char<_CharT2>::__value,
>> +                  _OutputIterator>::type
>> +    copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>> +#endif
>> +
>>       template<typename _CharT2>
>>         friend typename 
>> __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
>>                   istreambuf_iterator<_CharT2> >::__type
>> diff --git 
>> a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc 
>> b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>> new file mode 100644
>> index 00000000000..ebd769cf7c0
>> --- /dev/null
>> +++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>> @@ -0,0 +1,59 @@
>> +// Copyright (C) 2019 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 { target c++11 } }
>> +
>> +#include <algorithm>
>> +#include <sstream>
>> +#include <iterator>
>> +
>> +#include <testsuite_hooks.h>
>> +
>> +void test01()
>> +{
>> +  std::stringstream ss("12345");
>> +
>> +  std::string ostr(5, '0');
>> +  typedef std::istreambuf_iterator<char> istrb_ite;
>> +  std::copy_n(istrb_ite(ss), 0, ostr.begin());
>> +  VERIFY( ostr.front() == '0' );
>
> I'd like to see a check here that the value returned from copy_n is
> equal to ostr.begin().
>
>> +
>> +  std::copy_n(istrb_ite(ss), 2, ostr.begin());
>> +  VERIFY( ostr == "12000" );
>
> And equal to ostr.begin() + 2.
>
>> +
>> +  std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2);
>> +  VERIFY( ostr == "12345" );
>
> And equal to ostr.begin() + 5 here.
Done.
>
>> +}
>> +
>> +void test02()
>> +{
>> +  std::stringstream ss("12345");
>> +
>> +  std::string ostr(5, '0');
>> +  typedef std::istreambuf_iterator<char> istrb_ite;
>> +
>> +  istrb_ite ibfit(ss);
>> +  std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, ostr.begin()));
>> +  VERIFY( ostr == "12345" );
>> +}
>>
>
> Ideally I'd also like to see tests where the input buffer is larger
> than the size being read, e.g. read 5 chars from "123456" and verify
> we don't read the '6'.
In test01 I am doing something like that.
>
> Also, these tests don't exercise the code path that causes an
> underflow. It would be good to use an ifstream to read from one of the
> files in the testsuite/data directory, and read a large amount of data
> (more than fits in a filebuf's read area) so that the underflow logic
> is tested.

With this new proposal I don't need to do it, I'am counting on sgetn tests.

     * include/bits/stl_algo.h
     (__copy_n_a(_IIte, _Size, _OIte)): New.
     (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New declaration.
     (__copy_n(_IIte, _Size, _OIte, input_iterator_tag)): Adapt to use
     latter.
     * include/bits/streambuf_iterator.h (istreambuf_iterator<>): Declare
     std::__copy_n_a friend.
     (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New.
     * testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc: New.
     * testsuite/25_algorithms/copy_n/istreambuf_iterator/1_neg.cc: New.
     * testsuite/25_algorithms/copy_n/istreambuf_iterator/2_neg.cc: New.

Tested under Linux x86_64.

Ok to commit ?

François


[-- Attachment #2: istreambuf_copy_n.patch --]
[-- Type: text/x-patch, Size: 8536 bytes --]

diff --git a/libstdc++-v3/include/bits/stl_algo.h b/libstdc++-v3/include/bits/stl_algo.h
index 078efc028cc..ae4f41c53ff 100644
--- a/libstdc++-v3/include/bits/stl_algo.h
+++ b/libstdc++-v3/include/bits/stl_algo.h
@@ -778,14 +778,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _InputIterator, typename _Size, typename _OutputIterator>
     _GLIBCXX20_CONSTEXPR
     _OutputIterator
-    __copy_n(_InputIterator __first, _Size __n,
-	     _OutputIterator __result, input_iterator_tag)
+    __copy_n_a(_InputIterator __first, _Size __n, _OutputIterator __result)
     {
       for (; __n > 0; --__n, (void)++__first, (void)++__result)
 	*__result = *__first;
       return __result;
     }
 
+  template<typename _CharT, typename _Traits, typename _Size>
+    _CharT*
+    __copy_n_a(istreambuf_iterator<_CharT, _Traits>, _Size, _CharT*);
+
+  template<typename _InputIterator, typename _Size, typename _OutputIterator>
+    _GLIBCXX20_CONSTEXPR
+    _OutputIterator
+    __copy_n(_InputIterator __first, _Size __n,
+	     _OutputIterator __result, input_iterator_tag)
+    {
+      return std::__niter_wrap(__result,
+			       __copy_n_a(__first, __n,
+					  std::__niter_base(__result)));
+    }
+
   template<typename _RandomAccessIterator, typename _Size,
 	   typename _OutputIterator>
     _GLIBCXX20_CONSTEXPR
@@ -870,7 +884,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       return pair<_OutputIterator1, _OutputIterator2>(__out_true, __out_false);
     }
-#endif
+#endif // C++11
 
   template<typename _ForwardIterator, typename _Predicate>
     _GLIBCXX20_CONSTEXPR
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 2f4ff494a3a..b1b6bc129ce 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -80,6 +80,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	__copy_move_a2(istreambuf_iterator<_CharT2>,
 		       istreambuf_iterator<_CharT2>, _CharT2*);
 
+#if __cplusplus >= 201103L
+      template<typename _CharT2, typename _Traits2, typename _Size>
+	friend _CharT2*
+	__copy_n_a(istreambuf_iterator<_CharT2, _Traits2>, _Size, _CharT2*);
+#endif
+
       template<typename _CharT2>
 	friend typename __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
 				    istreambuf_iterator<_CharT2> >::__type
@@ -367,6 +373,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return __result;
     }
 
+#if __cplusplus >= 201103L
+  template<typename _CharT, typename _Traits, typename _Size>
+    _CharT*
+    __copy_n_a(istreambuf_iterator<_CharT, _Traits> __it,
+	       _Size __n, _CharT* __result)
+    {
+      if (__n == 0)
+	return __result;
+
+      __glibcxx_requires_cond(__it._M_sbuf,
+			      _M_message(__gnu_debug::__msg_inc_istreambuf)
+			      ._M_iterator(__it));
+      _CharT* __beg = __result;
+      __result += __it._M_sbuf->sgetn(__beg, __n);
+      __glibcxx_requires_cond(__result - __beg == __n,
+			      _M_message(__gnu_debug::__msg_inc_istreambuf)
+			      ._M_iterator(__it));
+      return __result;
+    }
+#endif // C++11
+
   template<typename _CharT>
     typename __gnu_cxx::__enable_if<__is_char<_CharT>::__value,
 		  		    istreambuf_iterator<_CharT> >::__type
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc
new file mode 100644
index 00000000000..87d37b6b9ca
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc
@@ -0,0 +1,73 @@
+// Copyright (C) 2019 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 { target c++11 } }
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  std::stringstream ss("12345");
+
+  std::string ostr(5, '0');
+  typedef std::istreambuf_iterator<char> istrb_ite;
+  auto res = std::copy_n(istrb_ite(ss), 0, ostr.begin());
+  VERIFY( res == ostr.begin() );
+  VERIFY( ostr.front() == '0' );
+
+  res = std::copy_n(istrb_ite(ss), 2, ostr.begin());
+  VERIFY( res == ostr.begin() + 2 );
+  VERIFY( ostr == "12000" );
+
+  res = std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2);
+  VERIFY( res == ostr.begin() + 5 );
+  VERIFY( ostr == "12345" );
+}
+
+void test02()
+{
+  std::stringstream ss("12345");
+
+  std::string ostr(5, '0');
+  typedef std::istreambuf_iterator<char> istrb_ite;
+
+  istrb_ite ibfit(ss);
+  auto res = std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, ostr.begin()));
+  VERIFY( res == ostr.begin() + 5 );
+  VERIFY( ostr == "12345" );
+}
+
+void test03()
+{
+  std::string ostr(5, '0');
+  typedef std::istreambuf_iterator<char> istrb_ite;
+
+  auto res = std::copy_n(istrb_ite(), 0, ostr.begin());
+  VERIFY( res == ostr.begin() );
+}
+
+int main()
+{
+  test01();
+  test02();
+  test03();
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1_neg.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1_neg.cc
new file mode 100644
index 00000000000..42bc5b64306
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1_neg.cc
@@ -0,0 +1,40 @@
+// Copyright (C) 2019 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 { target c++11 xfail *-*-* } }
+// { dg-require-debug-mode { } }
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  std::stringstream ss("12345");
+
+  std::string ostr(10, '0');
+  typedef std::istreambuf_iterator<char> istrb_ite;
+  std::copy_n(istrb_ite(ss), 10, ostr.begin());
+}
+
+int main()
+{
+  test01();
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/2_neg.cc b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/2_neg.cc
new file mode 100644
index 00000000000..cede27a248d
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/2_neg.cc
@@ -0,0 +1,38 @@
+// Copyright (C) 2019 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 { target c++11 xfail *-*-* } }
+// { dg-require-debug-mode { } }
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  std::string ostr(10, '0');
+  typedef std::istreambuf_iterator<char> istrb_ite;
+  std::copy_n(istrb_ite(), 10, ostr.begin());
+}
+
+int main()
+{
+  test01();
+  return 0;
+}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Add std::copy_n overload for istreambuf_iterator
  2019-10-04  5:01   ` François Dumont
@ 2019-10-04 11:06     ` Jonathan Wakely
  2019-10-09 20:18     ` Christophe Lyon
  1 sibling, 0 replies; 8+ messages in thread
From: Jonathan Wakely @ 2019-10-04 11:06 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

On 04/10/19 07:01 +0200, François Dumont wrote:
>On 9/27/19 1:00 PM, Jonathan Wakely wrote:
>>On 19/07/19 23:37 +0200, François Dumont wrote:
>>>It sounds reasonable to overload std::copy_n for istreambuf_iterator.
>>I wonder whether it's worth doing:
>>
>>#if __cplusplus >= 201703L
>>   if constexpr (is_same_v<_OutputIterator, _CharT*>)
>>     return __result + __it._M_sbuf->sgetn(__result, __n);
>>   else
>>     {
>>#endif
>>     ...
>>#if __cplusplus >= 201703L
>>     }
>>#endif
>>
>>We could extend that to also work for basic_string<_CharT>::iterator
>>and vector<_CharT>::iterator too if we wanted.
>>
>>I'm not sure if it will perform any better than the code below (it's
>>approximately equivalent) but it should result in smaller binaries, as we
>>wouldn't be instantiating the code below when outputting to a pointer
>>or contiguous iterator.
>>
>>We don't need to do that now, it can be a separate patch later (if we
>>do it at all).
>
>Very good remark, I hadn't check streambuf to find out if there were 
>better to do. For me it is the streambuf method to target for an 
>std::copy_n overload.
>
>So here is a new proposal much simpler. I see no reason to enable it 
>only for char types, is there ?

The reason to only do this for char-like types and std::char_traits is
that users could specialize istreambuf_iterator on their own types or
their own traits, and the specialization would not declare __copy_n_a
as a friend, and would not have the _M_sbuf member.

So I think we should still limit the optimisation to __is_char types
and std::char_traits. In practice that will help for all reasonable
cases, and unreasonable users who specialize the class can still
compile, but don't get the optimisation.


>>Ideally I'd also like to see tests where the input buffer is larger
>>than the size being read, e.g. read 5 chars from "123456" and verify
>>we don't read the '6'.
>In test01 I am doing something like that.

Thanks.

>>Also, these tests don't exercise the code path that causes an
>>underflow. It would be good to use an ifstream to read from one of the
>>files in the testsuite/data directory, and read a large amount of data
>>(more than fits in a filebuf's read area) so that the underflow logic
>>is tested.
>
>With this new proposal I don't need to do it, I'am counting on sgetn tests.

Agreed.

OK for trunk with the __is_char<_CharT> and char_traits<_CharT>
constraint restored. Thanks!


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Add std::copy_n overload for istreambuf_iterator
  2019-10-04  5:01   ` François Dumont
  2019-10-04 11:06     ` Jonathan Wakely
@ 2019-10-09 20:18     ` Christophe Lyon
  2019-10-09 20:29       ` François Dumont
  2019-10-10 19:57       ` François Dumont
  1 sibling, 2 replies; 8+ messages in thread
From: Christophe Lyon @ 2019-10-09 20:18 UTC (permalink / raw)
  To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches

On Fri, 4 Oct 2019 at 07:01, François Dumont <frs.dumont@gmail.com> wrote:

> On 9/27/19 1:00 PM, Jonathan Wakely wrote:
> > On 19/07/19 23:37 +0200, François Dumont wrote:
> >> It sounds reasonable to overload std::copy_n for istreambuf_iterator.
> > I wonder whether it's worth doing:
> >
> > #if __cplusplus >= 201703L
> >    if constexpr (is_same_v<_OutputIterator, _CharT*>)
> >      return __result + __it._M_sbuf->sgetn(__result, __n);
> >    else
> >      {
> > #endif
> >      ...
> > #if __cplusplus >= 201703L
> >      }
> > #endif
> >
> > We could extend that to also work for basic_string<_CharT>::iterator
> > and vector<_CharT>::iterator too if we wanted.
> >
> > I'm not sure if it will perform any better than the code below (it's
> > approximately equivalent) but it should result in smaller binaries, as we
> > wouldn't be instantiating the code below when outputting to a pointer
> > or contiguous iterator.
> >
> > We don't need to do that now, it can be a separate patch later (if we
> > do it at all).
>
> Very good remark, I hadn't check streambuf to find out if there were
> better to do. For me it is the streambuf method to target for an
> std::copy_n overload.
>
> So here is a new proposal much simpler. I see no reason to enable it
> only for char types, is there ?
>
> Once the other patch on copy/copy_backward... algos is in I'll provide
> what necessary to benefit from the same optimization for std::deque
> iterators and in Debug mode.
>
> >
> >> +#endif
> >
> > Because the matching #if is more than 40 lines away, please add a
> > comment noting the condition that this corresponds to, i.e.
> >
> > #endif // C++11
> Ok, done even if there is no 40 lines anymore. And also added it in
> stl_algo.h.
> >
> >> +
> >>   template<typename _CharT>
> >>     typename __gnu_cxx::__enable_if<__is_char<_CharT>::__value,
> >>                       istreambuf_iterator<_CharT> >::__type
> >> diff --git a/libstdc++-v3/include/std/streambuf
> >> b/libstdc++-v3/include/std/streambuf
> >> index d9ca981d704..4f62ebf4d95 100644
> >> --- a/libstdc++-v3/include/std/streambuf
> >> +++ b/libstdc++-v3/include/std/streambuf
> >> @@ -155,6 +155,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >>         __copy_move_a2(istreambuf_iterator<_CharT2>,
> >>                istreambuf_iterator<_CharT2>, _CharT2*);
> >>
> >> +#if __cplusplus >= 201103L
> >> +      template<typename _CharT2, typename _Size, typename
> >> _OutputIterator>
> >> +    friend typename enable_if<__is_char<_CharT2>::__value,
> >> +                  _OutputIterator>::type
> >> +    copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
> >> +#endif
> >> +
> >>       template<typename _CharT2>
> >>         friend typename
> >> __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
> >>                   istreambuf_iterator<_CharT2> >::__type
> >> diff --git
> >> a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
> >> b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
> >> new file mode 100644
> >> index 00000000000..ebd769cf7c0
> >> --- /dev/null
> >> +++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
> >> @@ -0,0 +1,59 @@
> >> +// Copyright (C) 2019 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 { target c++11 } }
> >> +
> >> +#include <algorithm>
> >> +#include <sstream>
> >> +#include <iterator>
> >> +
> >> +#include <testsuite_hooks.h>
> >> +
> >> +void test01()
> >> +{
> >> +  std::stringstream ss("12345");
> >> +
> >> +  std::string ostr(5, '0');
> >> +  typedef std::istreambuf_iterator<char> istrb_ite;
> >> +  std::copy_n(istrb_ite(ss), 0, ostr.begin());
> >> +  VERIFY( ostr.front() == '0' );
> >
> > I'd like to see a check here that the value returned from copy_n is
> > equal to ostr.begin().
> >
> >> +
> >> +  std::copy_n(istrb_ite(ss), 2, ostr.begin());
> >> +  VERIFY( ostr == "12000" );
> >
> > And equal to ostr.begin() + 2.
> >
> >> +
> >> +  std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2);
> >> +  VERIFY( ostr == "12345" );
> >
> > And equal to ostr.begin() + 5 here.
> Done.
> >
> >> +}
> >> +
> >> +void test02()
> >> +{
> >> +  std::stringstream ss("12345");
> >> +
> >> +  std::string ostr(5, '0');
> >> +  typedef std::istreambuf_iterator<char> istrb_ite;
> >> +
> >> +  istrb_ite ibfit(ss);
> >> +  std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, ostr.begin()));
> >> +  VERIFY( ostr == "12345" );
> >> +}
> >>
> >
> > Ideally I'd also like to see tests where the input buffer is larger
> > than the size being read, e.g. read 5 chars from "123456" and verify
> > we don't read the '6'.
> In test01 I am doing something like that.
> >
> > Also, these tests don't exercise the code path that causes an
> > underflow. It would be good to use an ifstream to read from one of the
> > files in the testsuite/data directory, and read a large amount of data
> > (more than fits in a filebuf's read area) so that the underflow logic
> > is tested.
>
> With this new proposal I don't need to do it, I'am counting on sgetn tests.
>
>      * include/bits/stl_algo.h
>      (__copy_n_a(_IIte, _Size, _OIte)): New.
>      (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New declaration.
>      (__copy_n(_IIte, _Size, _OIte, input_iterator_tag)): Adapt to use
>      latter.
>      * include/bits/streambuf_iterator.h (istreambuf_iterator<>): Declare
>      std::__copy_n_a friend.
>      (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New.
>      * testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc: New.
>

Hi,

Maybe this has already been reported, but I've noticed that this new test
fails on arm & aarch64.
My libstdc++.log says:
 /libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc:42:
void test01(): Assertion 'ostr == "12345"' failed.

Christophe

     * testsuite/25_algorithms/copy_n/istreambuf_iterator/1_neg.cc: New.
>      * testsuite/25_algorithms/copy_n/istreambuf_iterator/2_neg.cc: New.
>
> Tested under Linux x86_64.
>
> Ok to commit ?
>
> François
>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Add std::copy_n overload for istreambuf_iterator
  2019-10-09 20:18     ` Christophe Lyon
@ 2019-10-09 20:29       ` François Dumont
  2019-10-10 19:57       ` François Dumont
  1 sibling, 0 replies; 8+ messages in thread
From: François Dumont @ 2019-10-09 20:29 UTC (permalink / raw)
  To: Christophe Lyon; +Cc: Jonathan Wakely, libstdc++, gcc-patches

On 10/9/19 10:18 PM, Christophe Lyon wrote:
>
>
> On Fri, 4 Oct 2019 at 07:01, François Dumont <frs.dumont@gmail.com 
> <mailto:frs.dumont@gmail.com>> wrote:
>
>     On 9/27/19 1:00 PM, Jonathan Wakely wrote:
>     > On 19/07/19 23:37 +0200, François Dumont wrote:
>     >> It sounds reasonable to overload std::copy_n for
>     istreambuf_iterator.
>     > I wonder whether it's worth doing:
>     >
>     > #if __cplusplus >= 201703L
>     >    if constexpr (is_same_v<_OutputIterator, _CharT*>)
>     >      return __result + __it._M_sbuf->sgetn(__result, __n);
>     >    else
>     >      {
>     > #endif
>     >      ...
>     > #if __cplusplus >= 201703L
>     >      }
>     > #endif
>     >
>     > We could extend that to also work for basic_string<_CharT>::iterator
>     > and vector<_CharT>::iterator too if we wanted.
>     >
>     > I'm not sure if it will perform any better than the code below (it's
>     > approximately equivalent) but it should result in smaller
>     binaries, as we
>     > wouldn't be instantiating the code below when outputting to a
>     pointer
>     > or contiguous iterator.
>     >
>     > We don't need to do that now, it can be a separate patch later
>     (if we
>     > do it at all).
>
>     Very good remark, I hadn't check streambuf to find out if there were
>     better to do. For me it is the streambuf method to target for an
>     std::copy_n overload.
>
>     So here is a new proposal much simpler. I see no reason to enable it
>     only for char types, is there ?
>
>     Once the other patch on copy/copy_backward... algos is in I'll
>     provide
>     what necessary to benefit from the same optimization for std::deque
>     iterators and in Debug mode.
>
>     >
>     >> +#endif
>     >
>     > Because the matching #if is more than 40 lines away, please add a
>     > comment noting the condition that this corresponds to, i.e.
>     >
>     > #endif // C++11
>     Ok, done even if there is no 40 lines anymore. And also added it in
>     stl_algo.h.
>     >
>     >> +
>     >>   template<typename _CharT>
>     >>     typename __gnu_cxx::__enable_if<__is_char<_CharT>::__value,
>     >> istreambuf_iterator<_CharT> >::__type
>     >> diff --git a/libstdc++-v3/include/std/streambuf
>     >> b/libstdc++-v3/include/std/streambuf
>     >> index d9ca981d704..4f62ebf4d95 100644
>     >> --- a/libstdc++-v3/include/std/streambuf
>     >> +++ b/libstdc++-v3/include/std/streambuf
>     >> @@ -155,6 +155,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     >> __copy_move_a2(istreambuf_iterator<_CharT2>,
>     >>                istreambuf_iterator<_CharT2>, _CharT2*);
>     >>
>     >> +#if __cplusplus >= 201103L
>     >> +      template<typename _CharT2, typename _Size, typename
>     >> _OutputIterator>
>     >> +    friend typename enable_if<__is_char<_CharT2>::__value,
>     >> +                  _OutputIterator>::type
>     >> +    copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>     >> +#endif
>     >> +
>     >>       template<typename _CharT2>
>     >>         friend typename
>     >> __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
>     >> istreambuf_iterator<_CharT2> >::__type
>     >> diff --git
>     >>
>     a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>     >>
>     b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>     >> new file mode 100644
>     >> index 00000000000..ebd769cf7c0
>     >> --- /dev/null
>     >> +++
>     b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>     >> @@ -0,0 +1,59 @@
>     >> +// Copyright (C) 2019 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 { target c++11 } }
>     >> +
>     >> +#include <algorithm>
>     >> +#include <sstream>
>     >> +#include <iterator>
>     >> +
>     >> +#include <testsuite_hooks.h>
>     >> +
>     >> +void test01()
>     >> +{
>     >> +  std::stringstream ss("12345");
>     >> +
>     >> +  std::string ostr(5, '0');
>     >> +  typedef std::istreambuf_iterator<char> istrb_ite;
>     >> +  std::copy_n(istrb_ite(ss), 0, ostr.begin());
>     >> +  VERIFY( ostr.front() == '0' );
>     >
>     > I'd like to see a check here that the value returned from copy_n is
>     > equal to ostr.begin().
>     >
>     >> +
>     >> +  std::copy_n(istrb_ite(ss), 2, ostr.begin());
>     >> +  VERIFY( ostr == "12000" );
>     >
>     > And equal to ostr.begin() + 2.
>     >
>     >> +
>     >> +  std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2);
>     >> +  VERIFY( ostr == "12345" );
>     >
>     > And equal to ostr.begin() + 5 here.
>     Done.
>     >
>     >> +}
>     >> +
>     >> +void test02()
>     >> +{
>     >> +  std::stringstream ss("12345");
>     >> +
>     >> +  std::string ostr(5, '0');
>     >> +  typedef std::istreambuf_iterator<char> istrb_ite;
>     >> +
>     >> +  istrb_ite ibfit(ss);
>     >> +  std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, ostr.begin()));
>     >> +  VERIFY( ostr == "12345" );
>     >> +}
>     >>
>     >
>     > Ideally I'd also like to see tests where the input buffer is larger
>     > than the size being read, e.g. read 5 chars from "123456" and verify
>     > we don't read the '6'.
>     In test01 I am doing something like that.
>     >
>     > Also, these tests don't exercise the code path that causes an
>     > underflow. It would be good to use an ifstream to read from one
>     of the
>     > files in the testsuite/data directory, and read a large amount
>     of data
>     > (more than fits in a filebuf's read area) so that the underflow
>     logic
>     > is tested.
>
>     With this new proposal I don't need to do it, I'am counting on
>     sgetn tests.
>
>          * include/bits/stl_algo.h
>          (__copy_n_a(_IIte, _Size, _OIte)): New.
>          (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New
>     declaration.
>          (__copy_n(_IIte, _Size, _OIte, input_iterator_tag)): Adapt to use
>          latter.
>          * include/bits/streambuf_iterator.h (istreambuf_iterator<>):
>     Declare
>          std::__copy_n_a friend.
>          (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New.
>          * testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc: New.
>
>
> Hi,
>
> Maybe this has already been reported, but I've noticed that this new 
> test fails on arm & aarch64.
> My libstdc++.log says:
>  /libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc:42: 
> void test01(): Assertion 'ostr == "12345"' failed.
>
> Christophe
>
>          *
>     testsuite/25_algorithms/copy_n/istreambuf_iterator/1_neg.cc: New.
>          *
>     testsuite/25_algorithms/copy_n/istreambuf_iterator/2_neg.cc: New.
>
>     Tested under Linux x86_64.
>
>     Ok to commit ?
>
>     François
>
No, I wasn't aware about this issue, I'll have a look.

Thanks

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Add std::copy_n overload for istreambuf_iterator
  2019-10-09 20:18     ` Christophe Lyon
  2019-10-09 20:29       ` François Dumont
@ 2019-10-10 19:57       ` François Dumont
  2019-10-14 13:06         ` Christophe Lyon
  1 sibling, 1 reply; 8+ messages in thread
From: François Dumont @ 2019-10-10 19:57 UTC (permalink / raw)
  To: Christophe Lyon; +Cc: Jonathan Wakely, libstdc++, gcc-patches

I think this build has been done between the 2 commits I used to apply 
this patch (because of a problem between the chair and the keyboard). 
Now it should be fine.

But it shows that it changed the behavior of std::copy_n as soon as the 
new specialization is being used.

Without this change the result is "12234" whereas with the 
specialization it is "12345". So in addition to being a nice perf 
enhancement this patch was also a partial fix for PR 81857.

Let me know Jonathan if there is something to do about it.

François

On 10/9/19 10:18 PM, Christophe Lyon wrote:
>
>
> On Fri, 4 Oct 2019 at 07:01, François Dumont <frs.dumont@gmail.com 
> <mailto:frs.dumont@gmail.com>> wrote:
>
>     On 9/27/19 1:00 PM, Jonathan Wakely wrote:
>     > On 19/07/19 23:37 +0200, François Dumont wrote:
>     >> It sounds reasonable to overload std::copy_n for
>     istreambuf_iterator.
>     > I wonder whether it's worth doing:
>     >
>     > #if __cplusplus >= 201703L
>     >    if constexpr (is_same_v<_OutputIterator, _CharT*>)
>     >      return __result + __it._M_sbuf->sgetn(__result, __n);
>     >    else
>     >      {
>     > #endif
>     >      ...
>     > #if __cplusplus >= 201703L
>     >      }
>     > #endif
>     >
>     > We could extend that to also work for basic_string<_CharT>::iterator
>     > and vector<_CharT>::iterator too if we wanted.
>     >
>     > I'm not sure if it will perform any better than the code below (it's
>     > approximately equivalent) but it should result in smaller
>     binaries, as we
>     > wouldn't be instantiating the code below when outputting to a
>     pointer
>     > or contiguous iterator.
>     >
>     > We don't need to do that now, it can be a separate patch later
>     (if we
>     > do it at all).
>
>     Very good remark, I hadn't check streambuf to find out if there were
>     better to do. For me it is the streambuf method to target for an
>     std::copy_n overload.
>
>     So here is a new proposal much simpler. I see no reason to enable it
>     only for char types, is there ?
>
>     Once the other patch on copy/copy_backward... algos is in I'll
>     provide
>     what necessary to benefit from the same optimization for std::deque
>     iterators and in Debug mode.
>
>     >
>     >> +#endif
>     >
>     > Because the matching #if is more than 40 lines away, please add a
>     > comment noting the condition that this corresponds to, i.e.
>     >
>     > #endif // C++11
>     Ok, done even if there is no 40 lines anymore. And also added it in
>     stl_algo.h.
>     >
>     >> +
>     >>   template<typename _CharT>
>     >>     typename __gnu_cxx::__enable_if<__is_char<_CharT>::__value,
>     >> istreambuf_iterator<_CharT> >::__type
>     >> diff --git a/libstdc++-v3/include/std/streambuf
>     >> b/libstdc++-v3/include/std/streambuf
>     >> index d9ca981d704..4f62ebf4d95 100644
>     >> --- a/libstdc++-v3/include/std/streambuf
>     >> +++ b/libstdc++-v3/include/std/streambuf
>     >> @@ -155,6 +155,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     >> __copy_move_a2(istreambuf_iterator<_CharT2>,
>     >>                istreambuf_iterator<_CharT2>, _CharT2*);
>     >>
>     >> +#if __cplusplus >= 201103L
>     >> +      template<typename _CharT2, typename _Size, typename
>     >> _OutputIterator>
>     >> +    friend typename enable_if<__is_char<_CharT2>::__value,
>     >> +                  _OutputIterator>::type
>     >> +    copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>     >> +#endif
>     >> +
>     >>       template<typename _CharT2>
>     >>         friend typename
>     >> __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
>     >> istreambuf_iterator<_CharT2> >::__type
>     >> diff --git
>     >>
>     a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>     >>
>     b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>     >> new file mode 100644
>     >> index 00000000000..ebd769cf7c0
>     >> --- /dev/null
>     >> +++
>     b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>     >> @@ -0,0 +1,59 @@
>     >> +// Copyright (C) 2019 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 { target c++11 } }
>     >> +
>     >> +#include <algorithm>
>     >> +#include <sstream>
>     >> +#include <iterator>
>     >> +
>     >> +#include <testsuite_hooks.h>
>     >> +
>     >> +void test01()
>     >> +{
>     >> +  std::stringstream ss("12345");
>     >> +
>     >> +  std::string ostr(5, '0');
>     >> +  typedef std::istreambuf_iterator<char> istrb_ite;
>     >> +  std::copy_n(istrb_ite(ss), 0, ostr.begin());
>     >> +  VERIFY( ostr.front() == '0' );
>     >
>     > I'd like to see a check here that the value returned from copy_n is
>     > equal to ostr.begin().
>     >
>     >> +
>     >> +  std::copy_n(istrb_ite(ss), 2, ostr.begin());
>     >> +  VERIFY( ostr == "12000" );
>     >
>     > And equal to ostr.begin() + 2.
>     >
>     >> +
>     >> +  std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2);
>     >> +  VERIFY( ostr == "12345" );
>     >
>     > And equal to ostr.begin() + 5 here.
>     Done.
>     >
>     >> +}
>     >> +
>     >> +void test02()
>     >> +{
>     >> +  std::stringstream ss("12345");
>     >> +
>     >> +  std::string ostr(5, '0');
>     >> +  typedef std::istreambuf_iterator<char> istrb_ite;
>     >> +
>     >> +  istrb_ite ibfit(ss);
>     >> +  std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, ostr.begin()));
>     >> +  VERIFY( ostr == "12345" );
>     >> +}
>     >>
>     >
>     > Ideally I'd also like to see tests where the input buffer is larger
>     > than the size being read, e.g. read 5 chars from "123456" and verify
>     > we don't read the '6'.
>     In test01 I am doing something like that.
>     >
>     > Also, these tests don't exercise the code path that causes an
>     > underflow. It would be good to use an ifstream to read from one
>     of the
>     > files in the testsuite/data directory, and read a large amount
>     of data
>     > (more than fits in a filebuf's read area) so that the underflow
>     logic
>     > is tested.
>
>     With this new proposal I don't need to do it, I'am counting on
>     sgetn tests.
>
>          * include/bits/stl_algo.h
>          (__copy_n_a(_IIte, _Size, _OIte)): New.
>          (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New
>     declaration.
>          (__copy_n(_IIte, _Size, _OIte, input_iterator_tag)): Adapt to use
>          latter.
>          * include/bits/streambuf_iterator.h (istreambuf_iterator<>):
>     Declare
>          std::__copy_n_a friend.
>          (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New.
>          * testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc: New.
>
>
> Hi,
>
> Maybe this has already been reported, but I've noticed that this new 
> test fails on arm & aarch64.
> My libstdc++.log says:
>  /libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc:42: 
> void test01(): Assertion 'ostr == "12345"' failed.
>
> Christophe
>
>          *
>     testsuite/25_algorithms/copy_n/istreambuf_iterator/1_neg.cc: New.
>          *
>     testsuite/25_algorithms/copy_n/istreambuf_iterator/2_neg.cc: New.
>
>     Tested under Linux x86_64.
>
>     Ok to commit ?
>
>     François
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Add std::copy_n overload for istreambuf_iterator
  2019-10-10 19:57       ` François Dumont
@ 2019-10-14 13:06         ` Christophe Lyon
  0 siblings, 0 replies; 8+ messages in thread
From: Christophe Lyon @ 2019-10-14 13:06 UTC (permalink / raw)
  To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches

On Thu, 10 Oct 2019 at 21:57, François Dumont <frs.dumont@gmail.com> wrote:

> I think this build has been done between the 2 commits I used to apply
> this patch (because of a problem between the chair and the keyboard). Now
> it should be fine.
>
Indeed the test passes since at least r 276644
Thanks


> But it shows that it changed the behavior of std::copy_n as soon as the
> new specialization is being used.
>
> Without this change the result is "12234" whereas with the specialization
> it is "12345". So in addition to being a nice perf enhancement this patch
> was also a partial fix for PR 81857.
>
> Let me know Jonathan if there is something to do about it.
>
> François
>
> On 10/9/19 10:18 PM, Christophe Lyon wrote:
>
>
>
> On Fri, 4 Oct 2019 at 07:01, François Dumont <frs.dumont@gmail.com> wrote:
>
>> On 9/27/19 1:00 PM, Jonathan Wakely wrote:
>> > On 19/07/19 23:37 +0200, François Dumont wrote:
>> >> It sounds reasonable to overload std::copy_n for istreambuf_iterator.
>> > I wonder whether it's worth doing:
>> >
>> > #if __cplusplus >= 201703L
>> >    if constexpr (is_same_v<_OutputIterator, _CharT*>)
>> >      return __result + __it._M_sbuf->sgetn(__result, __n);
>> >    else
>> >      {
>> > #endif
>> >      ...
>> > #if __cplusplus >= 201703L
>> >      }
>> > #endif
>> >
>> > We could extend that to also work for basic_string<_CharT>::iterator
>> > and vector<_CharT>::iterator too if we wanted.
>> >
>> > I'm not sure if it will perform any better than the code below (it's
>> > approximately equivalent) but it should result in smaller binaries, as
>> we
>> > wouldn't be instantiating the code below when outputting to a pointer
>> > or contiguous iterator.
>> >
>> > We don't need to do that now, it can be a separate patch later (if we
>> > do it at all).
>>
>> Very good remark, I hadn't check streambuf to find out if there were
>> better to do. For me it is the streambuf method to target for an
>> std::copy_n overload.
>>
>> So here is a new proposal much simpler. I see no reason to enable it
>> only for char types, is there ?
>>
>> Once the other patch on copy/copy_backward... algos is in I'll provide
>> what necessary to benefit from the same optimization for std::deque
>> iterators and in Debug mode.
>>
>> >
>> >> +#endif
>> >
>> > Because the matching #if is more than 40 lines away, please add a
>> > comment noting the condition that this corresponds to, i.e.
>> >
>> > #endif // C++11
>> Ok, done even if there is no 40 lines anymore. And also added it in
>> stl_algo.h.
>> >
>> >> +
>> >>   template<typename _CharT>
>> >>     typename __gnu_cxx::__enable_if<__is_char<_CharT>::__value,
>> >>                       istreambuf_iterator<_CharT> >::__type
>> >> diff --git a/libstdc++-v3/include/std/streambuf
>> >> b/libstdc++-v3/include/std/streambuf
>> >> index d9ca981d704..4f62ebf4d95 100644
>> >> --- a/libstdc++-v3/include/std/streambuf
>> >> +++ b/libstdc++-v3/include/std/streambuf
>> >> @@ -155,6 +155,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >>         __copy_move_a2(istreambuf_iterator<_CharT2>,
>> >>                istreambuf_iterator<_CharT2>, _CharT2*);
>> >>
>> >> +#if __cplusplus >= 201103L
>> >> +      template<typename _CharT2, typename _Size, typename
>> >> _OutputIterator>
>> >> +    friend typename enable_if<__is_char<_CharT2>::__value,
>> >> +                  _OutputIterator>::type
>> >> +    copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>> >> +#endif
>> >> +
>> >>       template<typename _CharT2>
>> >>         friend typename
>> >> __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
>> >>                   istreambuf_iterator<_CharT2> >::__type
>> >> diff --git
>> >> a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>> >> b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>> >> new file mode 100644
>> >> index 00000000000..ebd769cf7c0
>> >> --- /dev/null
>> >> +++
>> b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>> >> @@ -0,0 +1,59 @@
>> >> +// Copyright (C) 2019 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 { target c++11 } }
>> >> +
>> >> +#include <algorithm>
>> >> +#include <sstream>
>> >> +#include <iterator>
>> >> +
>> >> +#include <testsuite_hooks.h>
>> >> +
>> >> +void test01()
>> >> +{
>> >> +  std::stringstream ss("12345");
>> >> +
>> >> +  std::string ostr(5, '0');
>> >> +  typedef std::istreambuf_iterator<char> istrb_ite;
>> >> +  std::copy_n(istrb_ite(ss), 0, ostr.begin());
>> >> +  VERIFY( ostr.front() == '0' );
>> >
>> > I'd like to see a check here that the value returned from copy_n is
>> > equal to ostr.begin().
>> >
>> >> +
>> >> +  std::copy_n(istrb_ite(ss), 2, ostr.begin());
>> >> +  VERIFY( ostr == "12000" );
>> >
>> > And equal to ostr.begin() + 2.
>> >
>> >> +
>> >> +  std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2);
>> >> +  VERIFY( ostr == "12345" );
>> >
>> > And equal to ostr.begin() + 5 here.
>> Done.
>> >
>> >> +}
>> >> +
>> >> +void test02()
>> >> +{
>> >> +  std::stringstream ss("12345");
>> >> +
>> >> +  std::string ostr(5, '0');
>> >> +  typedef std::istreambuf_iterator<char> istrb_ite;
>> >> +
>> >> +  istrb_ite ibfit(ss);
>> >> +  std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, ostr.begin()));
>> >> +  VERIFY( ostr == "12345" );
>> >> +}
>> >>
>> >
>> > Ideally I'd also like to see tests where the input buffer is larger
>> > than the size being read, e.g. read 5 chars from "123456" and verify
>> > we don't read the '6'.
>> In test01 I am doing something like that.
>> >
>> > Also, these tests don't exercise the code path that causes an
>> > underflow. It would be good to use an ifstream to read from one of the
>> > files in the testsuite/data directory, and read a large amount of data
>> > (more than fits in a filebuf's read area) so that the underflow logic
>> > is tested.
>>
>> With this new proposal I don't need to do it, I'am counting on sgetn
>> tests.
>>
>>      * include/bits/stl_algo.h
>>      (__copy_n_a(_IIte, _Size, _OIte)): New.
>>      (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New declaration.
>>      (__copy_n(_IIte, _Size, _OIte, input_iterator_tag)): Adapt to use
>>      latter.
>>      * include/bits/streambuf_iterator.h (istreambuf_iterator<>): Declare
>>      std::__copy_n_a friend.
>>      (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New.
>>      * testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc: New.
>>
>
> Hi,
>
> Maybe this has already been reported, but I've noticed that this new test
> fails on arm & aarch64.
> My libstdc++.log says:
>  /libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc:42:
> void test01(): Assertion 'ostr == "12345"' failed.
>
> Christophe
>
>      * testsuite/25_algorithms/copy_n/istreambuf_iterator/1_neg.cc: New.
>>      * testsuite/25_algorithms/copy_n/istreambuf_iterator/2_neg.cc: New.
>>
>> Tested under Linux x86_64.
>>
>> Ok to commit ?
>>
>> François
>>
>>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-10-14 13:06 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-19 21:38 Add std::copy_n overload for istreambuf_iterator François Dumont
2019-09-27 11:01 ` Jonathan Wakely
2019-10-04  5:01   ` François Dumont
2019-10-04 11:06     ` Jonathan Wakely
2019-10-09 20:18     ` Christophe Lyon
2019-10-09 20:29       ` François Dumont
2019-10-10 19:57       ` François Dumont
2019-10-14 13:06         ` Christophe Lyon

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).