Re: [Patch] Fix libstdc++/15002 (continued)

[Paolo Carlini <pcarlini at suse dot de>]

Pétur Runólfsson wrote:

> Hmmm... the existing tests for getline all seem to use stringbuf. This
> means that getline will always go exactly once through the loop, and
> always take the if (...) branch. It's probably a good idea to add a new
> test.
>
> There is an unbuffered streambuf that could be used to test this in
> testsuite/27_io/basic_istream/extractors_other/char/9424-in.cc
Agreed. You will find attached the updated patch. It would be nice to also
have a test going many times through the loop /and/ using traits::find/copy.
If nobody beats me I will prepare one soon, but the below seems now
sufficiently tested and reviewed.

> I'm not sure how much you need to worry about people specializing
> basic_streambuf. After all, the only reason __copy_streambufs works
> is because it is a friend of basic_streambuf, and I haven't seen
> many peoply complaining.
Right. In my mind, while summarizing what I have learned, I arrived at the
same conclusion. However, I'm not sure either we want to follow the bad
example of __copy_streambuf ;)

In such circumstances, I'd rather prefer implementing a quick dirty hack
(like mine of today ;) answering immediately user needs  *and* start working
together on the clean and general approach explained by Matt, which will
require some time.

> P.S. Here is a simple way to turn protected access into public without
> using friendship, from Andrei Alexandrescu:
This is very interesting and cute, as always happens with Andrei, but, 
again,
overall I don't think we should really go this kind of route! More or 
less it
would mean that we consider the library "... a badly designed framework...",
right? Also, Andrei says that "This still invokes undefined behavior, 
but the
risks are very low.". We must be /very/ careful these days: it looks like
more and more often the library is built also with compilers different from
g++ over which we don't have any control.

Paolo.

/////////////
2004-04-23  Paolo Carlini  <pcarlini@suse.de>
	    Petur Runolfsson  <peturr02@ru.is>

	PR libstdc++/15002 (continued)
	* include/bits/istream.tcc (basic_istream<>::getline(char_type*,
	streamsize, char_type)): Use traits::find/copy in a loop to speed
	up greatly the function in the common case (I/O buffer size >> 1).

2004-04-23  Paolo Carlini  <pcarlini@suse.de>

	* testsuite/27_io/basic_istream/getline/char/4.cc: New.

	* include/bits/istream.tcc (getline(basic_istream<>&,
	basic_string<>&, _CharT)): Change to use sgetc()/snextc() instead
	of sbumpc(), consistently with the other functions, thus also
	dealing correctly with the case of exceeded string::max_size().

diff -urN libstdc++-v3-orig/include/bits/istream.tcc libstdc++-v3/include/bits/istream.tcc
--- libstdc++-v3-orig/include/bits/istream.tcc	2004-02-08 05:46:41.000000000 +0100
+++ libstdc++-v3/include/bits/istream.tcc	2004-04-23 12:17:58.000000000 +0200
@@ -1,6 +1,6 @@
 // istream classes -*- C++ -*-
 
-// Copyright (C) 1997, 1998, 1999, 2000, 2001, 2002, 2003
+// Copyright (C) 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004
 // Free Software Foundation, Inc.
 //
 // This file is part of the GNU ISO C++ Library.  This library is free
@@ -592,27 +592,45 @@
 	      const int_type __eof = traits_type::eof();
 	      __streambuf_type* __sb = this->rdbuf();
 	      int_type __c = __sb->sgetc();
-
-	      while (_M_gcount + 1 < __n
+	      --__n;
+	      
+	      while (_M_gcount < __n
 		     && !traits_type::eq_int_type(__c, __eof)
 		     && !traits_type::eq_int_type(__c, __idelim))
 		{
-		  *__s++ = traits_type::to_char_type(__c);
-		  __c = __sb->snextc();
-		  ++_M_gcount;
+		  streamsize __size = std::min(streamsize(__sb->egptr()
+							  - __sb->gptr()),
+					       __n - _M_gcount);
+		  if (__size > 1)
+		    {
+		      const char_type* __p = traits_type::find(__sb->gptr(),
+							       __size,
+							       __delim);
+		      if (__p)
+			__size = __p - __sb->gptr();
+		      traits_type::copy(__s, __sb->gptr(), __size);
+		      __s += __size;
+		      __sb->gbump(__size);
+		      _M_gcount += __size;
+		      __c = __sb->sgetc();
+		    }
+		  else
+		    {
+		      *__s++ = traits_type::to_char_type(__c);
+		      __c = __sb->snextc();
+		      ++_M_gcount;
+		    }
 		}
+
 	      if (traits_type::eq_int_type(__c, __eof))
 		__err |= ios_base::eofbit;
-	      else
+	      else if (traits_type::eq_int_type(__c, __idelim))
 		{
-		  if (traits_type::eq_int_type(__c, __idelim))
-		    {
-		      __sb->sbumpc();
-		      ++_M_gcount;
-		    }
-		  else
-		    __err |= ios_base::failbit;
+		  __sb->sbumpc();
+		  ++_M_gcount;
 		}
+	      else
+		__err |= ios_base::failbit;
 	    }
 	  catch(...)
 	    { this->_M_setstate(ios_base::badbit); }
@@ -1085,22 +1103,28 @@
 	  try
 	    {
 	      __str.erase();
-	      __int_type __idelim = _Traits::to_int_type(__delim);
-	      __streambuf_type* __sb = __in.rdbuf();
-	      __int_type __c = __sb->sbumpc();
+	      const __int_type __idelim = _Traits::to_int_type(__delim);
 	      const __int_type __eof = _Traits::eof();
-	      __testdelim = _Traits::eq_int_type(__c, __idelim);
+	      __streambuf_type* __sb = __in.rdbuf();
+	      __int_type __c = __sb->sgetc();
 
-	      while (!_Traits::eq_int_type(__c, __eof) && !__testdelim
-		     && __extracted < __n)
+	      while (__extracted < __n
+		     && !_Traits::eq_int_type(__c, __eof)
+		     && !_Traits::eq_int_type(__c, __idelim))
 		{
 		  __str += _Traits::to_char_type(__c);
+		  __c = __sb->snextc();
 		  ++__extracted;
-		  __c = __sb->sbumpc();
-		  __testdelim = _Traits::eq_int_type(__c, __idelim);
 		}
 	      if (_Traits::eq_int_type(__c, __eof))
 		__err |= ios_base::eofbit;
+	      else if (_Traits::eq_int_type(__c, __idelim))
+		{
+		  __sb->sbumpc();
+		  ++__extracted;
+		}
+	      else
+		__err |= ios_base::failbit;
 	    }
 	  catch(...)
 	    {
@@ -1110,7 +1134,7 @@
 	      __in._M_setstate(ios_base::badbit);
 	    }
 	}
-      if ((!__extracted && !__testdelim) || __extracted == __n)
+      if (!__extracted)
 	__err |= ios_base::failbit;
       if (__err)
 	__in.setstate(__err);
diff -urN libstdc++-v3-orig/testsuite/27_io/basic_istream/getline/char/4.cc libstdc++-v3/testsuite/27_io/basic_istream/getline/char/4.cc
--- libstdc++-v3-orig/testsuite/27_io/basic_istream/getline/char/4.cc	1970-01-01 01:00:00.000000000 +0100
+++ libstdc++-v3/testsuite/27_io/basic_istream/getline/char/4.cc	2004-04-23 20:02:48.000000000 +0200
@@ -0,0 +1,106 @@
+// Copyright (C) 2004 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 2, 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 COPYING.  If not, write to the Free
+// Software Foundation, 59 Temple Place - Suite 330, Boston, MA 02111-1307,
+// USA.
+
+// As a special exception, you may use this file as part of a free software
+// library without restriction.  Specifically, if other files instantiate
+// templates or use macros or inline functions from this file, or you compile
+// this file and link it with other files to produce an executable, this
+// file does not by itself cause the resulting executable to be covered by
+// the GNU General Public License.  This exception does not however
+// invalidate any other reasons why the executable file might be covered by
+// the GNU General Public License.
+
+#include <sstream>
+#include <istream>
+#include <testsuite_hooks.h>
+
+class Inbuf : public std::streambuf
+{
+  static const char buf[];
+  const char* current;
+  int size;
+
+public:
+  Inbuf()
+  {
+    current = buf;
+    size = std::strlen(buf);
+  }
+  
+  int_type underflow()
+  {
+    if (current < buf + size)
+      return traits_type::to_int_type(*current);
+    return traits_type::eof();
+  }
+  
+  int_type uflow()
+  {
+    if (current < buf + size)
+      return traits_type::to_int_type(*current++);
+    return traits_type::eof();
+  }
+};
+
+const char Inbuf::buf[] = "1234567890abcdefghij";
+
+void test01()
+{
+  using namespace std;
+  bool test __attribute__((unused)) = true;
+
+  typedef char_traits<char>   traits_type;
+
+  Inbuf inbuf1;
+  istream is(&inbuf1);
+
+  char buffer[10];
+  traits_type::assign(buffer, sizeof(buffer), 'X');
+
+  is.getline(buffer, sizeof(buffer), '0');
+  VERIFY( is.rdstate() == ios_base::goodbit );
+  VERIFY( !traits_type::compare(buffer, "123456789\0", sizeof(buffer)) );
+  VERIFY( is.gcount() == 10 );
+
+  is.clear();
+  traits_type::assign(buffer, sizeof(buffer), 'X');
+  is.getline(buffer, sizeof(buffer));
+  VERIFY( is.rdstate() == ios_base::failbit );
+  VERIFY( !traits_type::compare(buffer, "abcdefghi\0", sizeof(buffer)) );
+  VERIFY( is.gcount() == 9 );
+
+  is.clear();
+  traits_type::assign(buffer, sizeof(buffer), 'X');
+  is.getline(buffer, sizeof(buffer));
+  VERIFY( is.rdstate() == ios_base::eofbit );
+  VERIFY( !traits_type::compare(buffer, "j\0XXXXXXXX", sizeof(buffer)) );
+  VERIFY( is.gcount() == 1 );
+
+  is.clear();
+  traits_type::assign(buffer, sizeof(buffer), 'X');
+  is.getline(buffer, sizeof(buffer));
+  VERIFY( is.rdstate() == (ios_base::eofbit | ios_base::failbit) );
+  VERIFY( !traits_type::compare(buffer, "\0XXXXXXXXX", sizeof(buffer)) );
+  VERIFY( is.gcount() == 0 );
+}
+
+int main() 
+{
+  test01();
+  return 0;
+}