diff mbox

std::quoted doesn't respect padding

Message ID 53909026.8020305@verizon.net
State New
Headers show

Commit Message

Ed Smith-Rowland June 5, 2014, 3:43 p.m. UTC
On 04/01/2014 07:33 AM, Jonathan Wakely wrote:
> [CCing gcc-patches]
>
> On 11/03/14 11:18 -0400, Ed Smith-Rowland wrote:
>> On 02/14/2014 07:56 PM, Jonathan Wakely wrote:
>>> We need to implement this fix (probably after 4.9 is released though)
>>>
>>> http://cplusplus.github.io/LWG/lwg-active.html#2344
>>>
>> Here is a patch (Stage 1 obviously).
>
> A couple of things I didn't notice earlier ...
>
>> Index: include/std/iomanip
>> ===================================================================
>> --- include/std/iomanip    (revision 208430)
>> +++ include/std/iomanip    (working copy)
>> @@ -41,6 +41,7 @@
>>
>> #if __cplusplus >= 201103L
>> #include <locale>
>> +#include <sstream> // used in quoted.
>
> We really only need <sstream> for __cplusplus > 201103L, otherwise we
> include it unnecessarily for C++11.
>
>>
>> -    return __os;
>> +    return __os << __ostr.str();
>>       }
>
> It should be slightly more efficient to do __os << __ostr.rdbuf() here,
> and in the other operator<< overload, since that copies directly from
> the stringbuf to __os's own streambuf, rather than creating a
> temporary std::string and copying from that.
>
>
Sorry for the hiatus...

Here is a new patch with issues fixed.

OK if it passes testing on x86_64-linux?

Ed
2014-06-05  Ed Smith-Rowland  <3dw4rd@verizon.net>

	DR 2344 - std::quoted doesn't respect padding
	* include/std/iomanip: Allow for padding in quoted inserters.
	* testsuite/27_io/manipulators/standard/char/dr2344.cc: New.
	* testsuite/27_io/manipulators/standard/wchar_t/dr2344.cc: New.

Comments

Jonathan Wakely June 5, 2014, 3:48 p.m. UTC | #1
On 05/06/14 11:43 -0400, Ed Smith-Rowland wrote:
>On 04/01/2014 07:33 AM, Jonathan Wakely wrote:
>>[CCing gcc-patches]
>>
>>On 11/03/14 11:18 -0400, Ed Smith-Rowland wrote:
>>>On 02/14/2014 07:56 PM, Jonathan Wakely wrote:
>>>>We need to implement this fix (probably after 4.9 is released though)
>>>>
>>>>http://cplusplus.github.io/LWG/lwg-active.html#2344
>>>>
>>>Here is a patch (Stage 1 obviously).
>>
>>A couple of things I didn't notice earlier ...
>>
>>>Index: include/std/iomanip
>>>===================================================================
>>>--- include/std/iomanip    (revision 208430)
>>>+++ include/std/iomanip    (working copy)
>>>@@ -41,6 +41,7 @@
>>>
>>>#if __cplusplus >= 201103L
>>>#include <locale>
>>>+#include <sstream> // used in quoted.
>>
>>We really only need <sstream> for __cplusplus > 201103L, otherwise we
>>include it unnecessarily for C++11.
>>
>>>
>>>-    return __os;
>>>+    return __os << __ostr.str();
>>>      }
>>
>>It should be slightly more efficient to do __os << __ostr.rdbuf() here,
>>and in the other operator<< overload, since that copies directly from
>>the stringbuf to __os's own streambuf, rather than creating a
>>temporary std::string and copying from that.
>>
>>
>Sorry for the hiatus...
>
>Here is a new patch with issues fixed.
>
>OK if it passes testing on x86_64-linux?

Great, OK for trunk and 4.9 too, I think.

Thanks!
Ed Smith-Rowland June 6, 2014, 2:08 p.m. UTC | #2
On 06/05/2014 11:48 AM, Jonathan Wakely wrote:
> On 05/06/14 11:43 -0400, Ed Smith-Rowland wrote:
>> On 04/01/2014 07:33 AM, Jonathan Wakely wrote:
>>> [CCing gcc-patches]
>>>
>>> On 11/03/14 11:18 -0400, Ed Smith-Rowland wrote:
>>>> On 02/14/2014 07:56 PM, Jonathan Wakely wrote:
>>>>> We need to implement this fix (probably after 4.9 is released though)
>>>>>
>>>>> http://cplusplus.github.io/LWG/lwg-active.html#2344
>>>>>
>>>> Here is a patch (Stage 1 obviously).
>>>
>>> A couple of things I didn't notice earlier ...
>>>
>>>> Index: include/std/iomanip
>>>> ===================================================================
>>>> --- include/std/iomanip    (revision 208430)
>>>> +++ include/std/iomanip    (working copy)
>>>> @@ -41,6 +41,7 @@
>>>>
>>>> #if __cplusplus >= 201103L
>>>> #include <locale>
>>>> +#include <sstream> // used in quoted.
>>>
>>> We really only need <sstream> for __cplusplus > 201103L, otherwise we
>>> include it unnecessarily for C++11.
>>>
>>>>
>>>> -    return __os;
>>>> +    return __os << __ostr.str();
>>>>      }
>>>
>>> It should be slightly more efficient to do __os << __ostr.rdbuf() here,
>>> and in the other operator<< overload, since that copies directly from
>>> the stringbuf to __os's own streambuf, rather than creating a
>>> temporary std::string and copying from that.
>>>
>>>
>> Sorry for the hiatus...
>>
>> Here is a new patch with issues fixed.
>>
>> OK if it passes testing on x86_64-linux?
>
> Great, OK for trunk and 4.9 too, I think.
>
> Thanks!
>
>
>
The rdbuf version fails.
The compare asserts fail and

   //  Should be: ["AB \"CD\" EF"xxxxxx]
   //  Gives    : ["AB \"CD\" EF"xxxxxx]
   std::cout << "[" << std::left << std::setfill('x') << std::setw(20) 
<< R"("AB \"CD\" EF")" << "]" << std::endl;
   //  Should be: ["GH \"IJ\" KL"yyyyyy]
   //  Gives    : ["yyyyyyyyyyyyyyyyyyyGH \"IJ\" KL"]
   std::cout << "[" << std::left << std::setfill('y') << std::setw(20) 
<< std::quoted(R"(GH "IJ" KL)") << "]" << std::endl;

This prints
["AB \"CD\" EF"xxxxxx]
[

No newline on the second line - just the '['.
I'll look at rdbuf. Error in rdbuf? Do I need to do something?

Failing that we know str() works even though it is inefficient.

Ed
Jonathan Wakely June 6, 2014, 2:27 p.m. UTC | #3
On 06/06/14 10:08 -0400, Ed Smith-Rowland wrote:
>I'll look at rdbuf. Error in rdbuf? Do I need to do something?

No, just me being dumb. The ostream::operator<<(streambuf*) overload
behaves as an unformatted output function, so won't obey the padding,
which is exactly what you're trying to fix.  Sorry for suggesting it.
diff mbox

Patch

Index: include/std/iomanip
===================================================================
--- include/std/iomanip	(revision 211281)
+++ include/std/iomanip	(working copy)
@@ -41,7 +41,10 @@ 
 
 #if __cplusplus >= 201103L
 #include <locale>
+#if __cplusplus > 201103L
+#include <sstream> // used in quoted.
 #endif
+#endif
 
 namespace std _GLIBCXX_VISIBILITY(default)
 {
@@ -342,7 +345,6 @@ 
 
     /**
      * @brief Struct for delimited strings.
-     *        The left and right delimiters can be different.
      */
     template<typename _String, typename _CharT>
       struct _Quoted_string
@@ -364,8 +366,10 @@ 
       };
 
     /**
-     * @brief Inserter for delimited strings.
-     *        The left and right delimiters can be different.
+     * @brief Inserter for quoted strings.
+     *
+     *  _GLIBCXX_RESOLVE_LIB_DEFECTS
+     *  DR 2344 quoted()'s interaction with padding is unclear
      */
     template<typename _CharT, typename _Traits>
       auto&
@@ -372,21 +376,24 @@ 
       operator<<(std::basic_ostream<_CharT, _Traits>& __os,
 		 const _Quoted_string<const _CharT*, _CharT>& __str)
       {
-	__os << __str._M_delim;
+	std::basic_ostringstream<_CharT, _Traits> __ostr;
+	__ostr << __str._M_delim;
 	for (const _CharT* __c = __str._M_string; *__c; ++__c)
 	  {
 	    if (*__c == __str._M_delim || *__c == __str._M_escape)
-	      __os << __str._M_escape;
-	    __os << *__c;
+	      __ostr << __str._M_escape;
+	    __ostr << *__c;
 	  }
-	__os << __str._M_delim;
+	__ostr << __str._M_delim;
 
-	return __os;
+	return __os << __ostr.rdbuf();
       }
 
     /**
-     * @brief Inserter for delimited strings.
-     *        The left and right delimiters can be different.
+     * @brief Inserter for quoted strings.
+     *
+     *  _GLIBCXX_RESOLVE_LIB_DEFECTS
+     *  DR 2344 quoted()'s interaction with padding is unclear
      */
     template<typename _CharT, typename _Traits, typename _String>
       auto&
@@ -393,16 +400,17 @@ 
       operator<<(std::basic_ostream<_CharT, _Traits>& __os,
 		 const _Quoted_string<_String, _CharT>& __str)
       {
-	__os << __str._M_delim;
+	std::basic_ostringstream<_CharT, _Traits> __ostr;
+	__ostr << __str._M_delim;
 	for (auto& __c : __str._M_string)
 	  {
 	    if (__c == __str._M_delim || __c == __str._M_escape)
-	      __os << __str._M_escape;
-	    __os << __c;
+	      __ostr << __str._M_escape;
+	    __ostr << __c;
 	  }
-	__os << __str._M_delim;
+	__ostr << __str._M_delim;
 
-	return __os;
+	return __os << __ostr.rdbuf();
       }
 
     /**
Index: testsuite/27_io/manipulators/standard/char/dr2344.cc
===================================================================
--- testsuite/27_io/manipulators/standard/char/dr2344.cc	(revision 0)
+++ testsuite/27_io/manipulators/standard/char/dr2344.cc	(working copy)
@@ -0,0 +1,50 @@ 
+// { dg-do run }
+// { dg-options "-std=gnu++14" }
+
+// Copyright (C) 2014 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/>.
+
+// 27.7.6 - Quoted manipulators		[quoted.manip]
+
+#include <sstream>
+#include <iomanip>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  bool test [[gnu::unused]] = true;
+
+  std::ostringstream ssx;
+  ssx << "[" << std::left << std::setfill('x') << std::setw(20) << R"("AB \"CD\" EF")" << "]";
+  VERIFY( ssx.str() == R"(["AB \"CD\" EF"xxxxxx])" );
+
+  std::ostringstream ssy;
+  ssy << "[" << std::left << std::setfill('y') << std::setw(20) << std::quoted(R"(GH "IJ" KL)") << "]";
+  VERIFY( ssy.str() == R"(["GH \"IJ\" KL"yyyyyy])" );
+
+  std::ostringstream ssz;
+  ssz << "[" << std::right << std::setfill('z') << std::setw(20) << std::quoted(R"(PQ "RS" TU)") << "]";
+  VERIFY( ssz.str() == R"([zzzzzz"PQ \"RS\" TU"])" );
+}
+
+int
+main()
+{
+  test01();
+  return 0;
+}
Index: testsuite/27_io/manipulators/standard/wchar_t/dr2344.cc
===================================================================
--- testsuite/27_io/manipulators/standard/wchar_t/dr2344.cc	(revision 0)
+++ testsuite/27_io/manipulators/standard/wchar_t/dr2344.cc	(working copy)
@@ -0,0 +1,50 @@ 
+// { dg-do run }
+// { dg-options "-std=gnu++14" }
+
+// Copyright (C) 2014 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/>.
+
+// 27.7.6 - Quoted manipulators		[quoted.manip]
+
+#include <sstream>
+#include <iomanip>
+#include <testsuite_hooks.h>
+
+void
+test01()
+{
+  bool test [[gnu::unused]] = true;
+
+  std::wostringstream ssx;
+  ssx << L"[" << std::left << std::setfill(L'x') << std::setw(20) << LR"("AB \"CD\" EF")" << L"]";
+  VERIFY( ssx.str() == LR"(["AB \"CD\" EF"xxxxxx])" );
+
+  std::wostringstream ssy;
+  ssy << L"[" << std::left << std::setfill(L'y') << std::setw(20) << std::quoted(LR"(GH "IJ" KL)") << L"]";
+  VERIFY( ssy.str() == LR"(["GH \"IJ\" KL"yyyyyy])" );
+
+  std::wostringstream ssz;
+  ssz << L"[" << std::right << std::setfill(L'z') << std::setw(20) << std::quoted(LR"(PQ "RS" TU)") << L"]";
+  VERIFY( ssz.str() == LR"([zzzzzz"PQ \"RS\" TU"])" );
+}
+
+int
+main()
+{
+  test01();
+  return 0;
+}