PR libstdc++/59568 fix error handling for std::complex stream extraction

Message ID 20171213184256.GA9171@redhat.com
State New
Headers show
Series
  • PR libstdc++/59568 fix error handling for std::complex stream extraction
Related show

Commit Message

Jonathan Wakely Dec. 13, 2017, 6:42 p.m.
The bug here is that we called putback even if the initial __is >> __ch
extraction failed and set eofbit, and putback clears the eofbit. I
found a number of other problems though, such as not even trying to
call putback after failing to find the ',' and ')' characters.

I decided to rewrite the function following the proposed resolution
for https://wg21.link/lwg2714 which is a much more precise
specification for much more desirable semantics.

	PR libstdc++/59568
	* include/std/complex (operator>>): Implement proposed resolution to
	LWG 2714. Use putback if and only if a character has been successfully
	extracted but isn't a delimiter. Use ctype::widen and traits::eq when
	testing if extracted characters match delimiters.
	* testsuite/26_numerics/complex/dr2714.cc: New test.

Tested powerpc64le-linux, committed to trunk.

For the release branches I'm considering just fixing the bug that
clears eofbit, and not the whole rewrite of the function.
commit 419381b5d32b5a38c1fe7703dc0400c836106939
Author: redi <redi@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Wed Dec 13 17:02:14 2017 +0000

    PR libstdc++/59568 fix error handling for std::complex stream extraction
    
            PR libstdc++/59568
            * include/std/complex (operator>>): Implement proposed resolution to
            LWG 2714. Use putback if and only if a character has been successfully
            extracted but isn't a delimiter. Use ctype::widen and traits::eq when
            testing if extracted characters match delimiters.
            * testsuite/26_numerics/complex/dr2714.cc: New test.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@255608 138bc75d-0d04-0410-961f-82ee72b054a4

Comments

Jonathan Wakely Dec. 13, 2017, 9:23 p.m. | #1
On 13/12/17 18:42 +0000, Jonathan Wakely wrote:
>The bug here is that we called putback even if the initial __is >> __ch

>extraction failed and set eofbit, and putback clears the eofbit. I

>found a number of other problems though, such as not even trying to

>call putback after failing to find the ',' and ')' characters.

>

>I decided to rewrite the function following the proposed resolution

>for https://wg21.link/lwg2714 which is a much more precise

>specification for much more desirable semantics.

>

>	PR libstdc++/59568

>	* include/std/complex (operator>>): Implement proposed resolution to

>	LWG 2714. Use putback if and only if a character has been successfully

>	extracted but isn't a delimiter. Use ctype::widen and traits::eq when

>	testing if extracted characters match delimiters.

>	* testsuite/26_numerics/complex/dr2714.cc: New test.

>

>Tested powerpc64le-linux, committed to trunk.

>

>For the release branches I'm considering just fixing the bug that

>clears eofbit, and not the whole rewrite of the function.


Actually there's another bug in the original function, which is that
it unconditionally sets "__x = __re_x;" even if extracting a value
into __re_x failed.

Here's what I plan to commit for the branches.

Tested x86_64-linux.
commit 907f186cd5d2e98f8ddf031b46b2cb0ae520b0d7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Dec 13 20:21:26 2017 +0000

    PR libstdc++/59568 don't use putback or update value when extraction fails
    
            PR libstdc++/59568
            * include/std/complex (operator>>): Only use putback if a character
            was successfully extracted and only set the value if a number was
            successfully extracted.
            * testsuite/26_numerics/complex/inserters_extractors/char/59568.cc:
            New test.

diff --git a/libstdc++-v3/include/std/complex b/libstdc++-v3/include/std/complex
index 6342c98e88a..22107cb2264 100644
--- a/libstdc++-v3/include/std/complex
+++ b/libstdc++-v3/include/std/complex
@@ -493,7 +493,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     operator>>(basic_istream<_CharT, _Traits>& __is, complex<_Tp>& __x)
     {
       _Tp __re_x, __im_x;
-      _CharT __ch;
+      _CharT __ch = _CharT();
       __is >> __ch;
       if (__ch == '(')
 	{
@@ -511,11 +511,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  else
 	    __is.setstate(ios_base::failbit);
 	}
-      else
+      else if (__is)
 	{
 	  __is.putback(__ch);
-	  __is >> __re_x;
-	  __x = __re_x;
+	  if (__is >> __re_x)
+	    __x = __re_x;
+	  else
+	    __is.setstate(ios_base::failbit);
 	}
       return __is;
     }
diff --git a/libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/59568.cc b/libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/59568.cc
new file mode 100644
index 00000000000..2bbdb6abae4
--- /dev/null
+++ b/libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/59568.cc
@@ -0,0 +1,166 @@
+// Copyright (C) 2017 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-options "-std=gnu++98" }
+
+#include <complex>
+#include <sstream>
+#include <complex>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  std::istringstream in(" 1 (2) ( 2.0 , 0.5 ) ");
+  std::complex<double> c1, c2, c3;
+  in >> c1 >> c2 >> c3;
+  VERIFY( in.good() );
+  VERIFY( c1.real() == 1 && c1.imag() == 0 );
+  VERIFY( c2.real() == 2 && c2.imag() == 0 );
+  VERIFY( c3.real() == 2 && c3.imag() == 0.5 );
+}
+
+void
+test02()
+{
+  std::istringstream in;
+  std::complex<double> c(-1, -1);
+  const std::complex<double> c0 = c;
+
+  in.str("a");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'a' );
+  VERIFY( c == c0 );
+
+  in.str(" ( ) ");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == ')' );
+  VERIFY( c == c0 );
+
+  in.str("(,");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == ',' );
+  VERIFY( c == c0 );
+
+  in.str("(b)");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'b' );
+  VERIFY( c == c0 );
+
+  in.str("( c)");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'c' );
+  VERIFY( c == c0 );
+
+  in.str("(99d");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  // VERIFY( in.get() == 'd' );
+  VERIFY( c == c0 );
+
+  in.str("(99 e");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  // VERIFY( in.get() == 'e' );
+  VERIFY( c == c0 );
+
+  in.str("(99, f");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'f' );
+  VERIFY( c == c0 );
+
+  in.str("(99, 88g");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  // VERIFY( in.get() == 'g' );
+  VERIFY( c == c0 );
+
+  in.str("(99, 88 h");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  // VERIFY( in.get() == 'h' );
+  VERIFY( c == c0 );
+
+  in.str("(99, )");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == ')' );
+  VERIFY( c == c0 );
+}
+
+void
+test03()
+{
+  // PR libstdc++/59568
+  std::istringstream in;
+  std::complex<double> c;
+
+  in.str("");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+
+  in.str(" ");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+
+  in.str("(99");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+
+  in.str("(99,");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+
+  in.str("(99,99");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+}
+
+int
+main()
+{
+  test01();
+  test02();
+  test03();
+}
Jonathan Wakely Dec. 14, 2017, 11:28 a.m. | #2
On 13/12/17 18:42 +0000, Jonathan Wakely wrote:
>The bug here is that we called putback even if the initial __is >> __ch

>extraction failed and set eofbit, and putback clears the eofbit. I

>found a number of other problems though, such as not even trying to

>call putback after failing to find the ',' and ')' characters.

>

>I decided to rewrite the function following the proposed resolution

>for https://wg21.link/lwg2714 which is a much more precise

>specification for much more desirable semantics.

>

>	PR libstdc++/59568

>	* include/std/complex (operator>>): Implement proposed resolution to

>	LWG 2714. Use putback if and only if a character has been successfully

>	extracted but isn't a delimiter. Use ctype::widen and traits::eq when

>	testing if extracted characters match delimiters.

>	* testsuite/26_numerics/complex/dr2714.cc: New test.

>

>Tested powerpc64le-linux, committed to trunk.


Here's a tweak for the testcase. Tested x86_64-linux, committed to trunk.
commit 4b8abc556e9ae9d4ff76f0b446eadf4a6f216638
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Dec 13 19:47:19 2017 +0000

    Improve std::complex test and move to sub-directory
    
            * testsuite/26_numerics/complex/dr2714.cc: Move to ...
            * testsuite/26_numerics/complex/inserters_extractors/char/dr2714.cc:
            ... Here. Remove duplicate header and dg-options. Check first invalid
            character gets putback. Remove wchar_t test.

diff --git a/libstdc++-v3/testsuite/26_numerics/complex/dr2714.cc b/libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/dr2714.cc
similarity index 92%
rename from libstdc++-v3/testsuite/26_numerics/complex/dr2714.cc
rename to libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/dr2714.cc
+++ b/libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/dr2714.cc
@@ -15,11 +15,8 @@
 // with this library; see the file COPYING3.  If not see
 // <http://www.gnu.org/licenses/>.
 
-// { dg-options "-std=gnu++98" }
-
 #include <complex>
 #include <sstream>
-#include <complex>
 #include <testsuite_hooks.h>
 
 void
@@ -37,16 +34,6 @@ test01()
 void
 test02()
 {
-  std::wistringstream in(L" ( 2.0 , 0.5 ) ");
-  std::complex<double> c;
-  in >> c;
-  VERIFY( in.good() );
-  VERIFY( c.real() == 2.0 && c.imag() == 0.5 );
-}
-
-void
-test03()
-{
   std::istringstream in;
   std::complex<double> c(-1, -1);
   const std::complex<double> c0 = c;
@@ -55,6 +42,7 @@ test03()
   in >> c;
   VERIFY( in.fail() );
   in.clear();
+  VERIFY( in.get() == 'a' );
 
   in.str(" ( ) ");
   in >> c;
@@ -71,11 +59,10 @@ test03()
   in.str("(b)");
   in >> c;
   VERIFY( in.fail() );
-
   in.clear();
   VERIFY( in.get() == 'b' );
-  in.str("( c)");
 
+  in.str("( c)");
   in >> c;
   VERIFY( in.fail() );
   in.clear();
@@ -121,7 +108,7 @@ test03()
 }
 
 void
-test04()
+test03()
 {
   // PR libstdc++/59568
   std::istringstream in;
@@ -164,5 +151,4 @@ main()
   test01();
   test02();
   test03();
-  test04();
 }
Jonathan Wakely Dec. 14, 2017, 11:48 a.m. | #3
On 14/12/17 11:28 +0000, Jonathan Wakely wrote:
>On 13/12/17 18:42 +0000, Jonathan Wakely wrote:

>>The bug here is that we called putback even if the initial __is >> __ch

>>extraction failed and set eofbit, and putback clears the eofbit. I

>>found a number of other problems though, such as not even trying to

>>call putback after failing to find the ',' and ')' characters.

>>

>>I decided to rewrite the function following the proposed resolution

>>for https://wg21.link/lwg2714 which is a much more precise

>>specification for much more desirable semantics.

>>

>>	PR libstdc++/59568

>>	* include/std/complex (operator>>): Implement proposed resolution to

>>	LWG 2714. Use putback if and only if a character has been successfully

>>	extracted but isn't a delimiter. Use ctype::widen and traits::eq when

>>	testing if extracted characters match delimiters.

>>	* testsuite/26_numerics/complex/dr2714.cc: New test.

>>

>>Tested powerpc64le-linux, committed to trunk.

>

>Here's a tweak for the testcase. Tested x86_64-linux, committed to trunk.


And an extra test for whitespace handling. Committed to trunk.
commit fdcb019de0eab2e6b3d0844422e8984bf5d95622
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Dec 14 11:41:43 2017 +0000

    Test whitespace handling in std::complex extraction
    
            * testsuite/26_numerics/complex/inserters_extractors/char/dr2714.cc:
            Add tests using noskipws.

diff --git a/libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/dr2714.cc b/libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/dr2714.cc
index 17fb8a249d9..952c52f4a2b 100644
--- a/libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/dr2714.cc
+++ b/libstdc++-v3/testsuite/26_numerics/complex/inserters_extractors/char/dr2714.cc
@@ -145,10 +145,38 @@ test03()
   in.clear();
 }
 
+void
+test04()
+{
+  // Test noskipws handling
+  std::istringstream in;
+  const char* bad_inputs[] = {
+    " 1", " (2)", "( 2)", "(2 )", "(2 ,3)", "(2,3 )", 0
+  };
+  const std::complex<double> c0(-1, -1);
+  std::complex<double> c;
+  for (int i = 0; bad_inputs[i]; ++i)
+  {
+    c = c0;
+    in.clear();
+    in.str(bad_inputs[i]);
+    in >> std::noskipws >> c;
+    VERIFY( in.fail() );
+    VERIFY( c == c0 );
+
+    in.clear();
+    in.str(bad_inputs[i]);
+    in >> std::skipws >> c;
+    VERIFY( !in.fail() );
+    VERIFY( c != c0 );
+  }
+}
+
 int
 main()
 {
   test01();
   test02();
   test03();
+  test04();
 }

Patch

diff --git a/libstdc++-v3/include/std/complex b/libstdc++-v3/include/std/complex
index 61f8cc1fce3..bfe10347bd3 100644
--- a/libstdc++-v3/include/std/complex
+++ b/libstdc++-v3/include/std/complex
@@ -492,31 +492,52 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     basic_istream<_CharT, _Traits>&
     operator>>(basic_istream<_CharT, _Traits>& __is, complex<_Tp>& __x)
     {
-      _Tp __re_x, __im_x;
+      bool __fail = true;
       _CharT __ch;
-      __is >> __ch;
-      if (__ch == '(')
+      if (__is >> __ch)
 	{
-	  __is >> __re_x >> __ch;
-	  if (__ch == ',')
+	  if (_Traits::eq(__ch, __is.widen('(')))
 	    {
-	      __is >> __im_x >> __ch;
-	      if (__ch == ')')
-		__x = complex<_Tp>(__re_x, __im_x);
-	      else
-		__is.setstate(ios_base::failbit);
+	      _Tp __u;
+	      if (__is >> __u >> __ch)
+		{
+		  const _CharT __rparen = __is.widen(')');
+		  if (_Traits::eq(__ch, __rparen))
+		    {
+		      __x = __u;
+		      __fail = false;
+		    }
+		  else if (_Traits::eq(__ch, __is.widen(',')))
+		    {
+		      _Tp __v;
+		      if (__is >> __v >> __ch)
+			{
+			  if (_Traits::eq(__ch, __rparen))
+			    {
+			      __x = complex<_Tp>(__u, __v);
+			      __fail = false;
+			    }
+			  else
+			    __is.putback(__ch);
+			}
+		    }
+		  else
+		    __is.putback(__ch);
+		}
 	    }
-	  else if (__ch == ')')
-	    __x = __re_x;
 	  else
-	    __is.setstate(ios_base::failbit);
-	}
-      else
-	{
-	  __is.putback(__ch);
-	  __is >> __re_x;
-	  __x = __re_x;
+	    {
+	      __is.putback(__ch);
+	      _Tp __u;
+	      if (__is >> __u)
+		{
+		  __x = __u;
+		  __fail = false;
+		}
+	    }
 	}
+      if (__fail)
+	__is.setstate(ios_base::failbit);
       return __is;
     }
 
diff --git a/libstdc++-v3/testsuite/26_numerics/complex/dr2714.cc b/libstdc++-v3/testsuite/26_numerics/complex/dr2714.cc
new file mode 100644
index 00000000000..6b35e8adcf9
--- /dev/null
+++ b/libstdc++-v3/testsuite/26_numerics/complex/dr2714.cc
@@ -0,0 +1,168 @@ 
+// Copyright (C) 2017 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-options "-std=gnu++98" }
+
+#include <complex>
+#include <sstream>
+#include <complex>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  std::istringstream in(" 1 (2) ( 2.0 , 0.5 ) ");
+  std::complex<double> c1, c2, c3;
+  in >> c1 >> c2 >> c3;
+  VERIFY( in.good() );
+  VERIFY( c1.real() == 1 && c1.imag() == 0 );
+  VERIFY( c2.real() == 2 && c2.imag() == 0 );
+  VERIFY( c3.real() == 2 && c3.imag() == 0.5 );
+}
+
+void
+test02()
+{
+  std::wistringstream in(L" ( 2.0 , 0.5 ) ");
+  std::complex<double> c;
+  in >> c;
+  VERIFY( in.good() );
+  VERIFY( c.real() == 2.0 && c.imag() == 0.5 );
+}
+
+void
+test03()
+{
+  std::istringstream in;
+  std::complex<double> c(-1, -1);
+  const std::complex<double> c0 = c;
+
+  in.str("a");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+
+  in.str(" ( ) ");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == ')' );
+
+  in.str("(,");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == ',' );
+
+  in.str("(b)");
+  in >> c;
+  VERIFY( in.fail() );
+
+  in.clear();
+  VERIFY( in.get() == 'b' );
+  in.str("( c)");
+
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'c' );
+
+  in.str("(99d");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'd' );
+
+  in.str("(99 e");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'e' );
+
+  in.str("(99, f");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'f' );
+
+  in.str("(99, 88g");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'g' );
+
+  in.str("(99, 88 h");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == 'h' );
+
+  in.str("(99, )");
+  in >> c;
+  VERIFY( in.fail() );
+  in.clear();
+  VERIFY( in.get() == ')' );
+
+  VERIFY( c == c0 );
+}
+
+void
+test04()
+{
+  // PR libstdc++/59568
+  std::istringstream in;
+  std::complex<double> c;
+
+  in.str("");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+
+  in.str(" ");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+
+  in.str("(99");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+
+  in.str("(99,");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+
+  in.str("(99,99");
+  in >> c;
+  VERIFY( in.fail() );
+  VERIFY( in.eof() );
+  in.clear();
+}
+
+int
+main()
+{
+  test01();
+  test02();
+  test03();
+  test04();
+}