Re: [committed] libstdc++: Implement LWG 1203 for rvalue iostreams

[Jonathan Wakely <jwakely@redhat.com>]

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

On 06/05/21 18:28 +0100, Jonathan Wakely wrote:
>On 06/05/21 18:09 +0100, Jonathan Wakely wrote:
>>On 06/05/21 17:55 +0200, Stephan Bergmann wrote:
>>>On 30/04/2021 15:48, Jonathan Wakely via Libstdc++ wrote:
>>>>This implements the resolution of LWG 1203 so that the constraints for
>>>>rvalue stream insertion/extraction are simpler, and the return type is
>>>>the original rvalue stream type not its base class.
>>>>
>>>>Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
>>>>
>>>>libstdc++-v3/ChangeLog:
>>>>
>>>>	* include/std/istream (operator>>(Istream&&, x&)): Simplify, as
>>>>	per LWG 1203.
>>>>	* include/std/ostream (operator<<(Ostream&&, const x&)):
>>>>	Likewise.
>>>>	* testsuite/27_io/basic_istream/extractors_character/char/lwg2499_neg.cc:
>>>>	Adjust dg-error pattern.
>>>>	* testsuite/27_io/basic_istream/extractors_character/wchar_t/lwg2499_neg.cc:
>>>>	Likewise.
>>>>	* testsuite/27_io/basic_istream/extractors_other/char/4.cc: Define
>>>>	is_extractable trait to replace std::__is_extractable. Make it
>>>>	work with rvalue streams as well as lvalues, to replace f() and
>>>>	g() helper functions.
>>>>	* testsuite/27_io/basic_istream/extractors_other/wchar_t/4.cc:
>>>>	Likewise.
>>>>	* testsuite/27_io/basic_ostream/inserters_other/char/6.cc:
>>>>	Define is_insertable trait to replace std::__is_insertable. Make
>>>>	it work with rvalue streams as well as lvalues, to replace f()
>>>>	and g() helper functions.
>>>>	* testsuite/27_io/basic_ostream/inserters_other/wchar_t/6.cc:
>>>>	Likewise.
>>>>	* testsuite/27_io/filesystem/path/io/dr2989.cc: Prune additional
>>>>	errors from new constraints.
>>>>	* testsuite/27_io/rvalue_streams-2.cc: Remove PR 80675 checks,
>>>>	which are no longer expected to compile.
>>>>	* testsuite/27_io/rvalue_streams.cc: Adjust existing test.
>>>>	Verify LWG 1203 changes.
>>>>
>>>>Tested powerpc64le-linux. Committed to trunk.
>>>
>>>FWIW, it looks like this is causing issues for Clang (at least 
>>>Clang 11 and recent Clang 13 trunk):
>>>
>>>>$ cat test.cc
>>>>#include <ostream>
>>>>int i = 1 << std::ios::erase_event;
>>>
>>>(i.e., using and enum in namespace std),
>>>
>>>>$ clang++ --gcc-toolchain=~/gcc/trunk/inst -fsyntax-only test.cc
>>>>In file included from test.cc:1:
>>>>~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/12.0.0/../../../../include/c++/12.0.0/ostream:727:33: error: cannot initialize a parameter of type 'std::ios_base *' with an rvalue of type 'int *'
>>>>  __rval_streamable(ios_base* = (_Tp*)nullptr);
>>>>                              ^ ~~~~~~~~~~~~~
>>>>~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/12.0.0/../../../../include/c++/12.0.0/ostream:733:25: note: in instantiation of default function argument expression for '__rval_streamable<int, void>' required here
>>>>         typename = decltype(std::__rval_streamable<_Os>()
>>>>                             ^
>>>>~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/12.0.0/../../../../include/c++/12.0.0/ostream:748:12: note: in instantiation of default argument for '__rvalue_stream_insertion_t<int, std::ios_base::event>' required here
>>>>  inline __rvalue_stream_insertion_t<_Ostream, _Tp>
>>>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>test.cc:2:11: note: while substituting deduced template arguments into function template 'operator<<' [with _Ostream = int, _Tp = std::ios_base::event]
>>>>int i = 1 << std::ios::erase_event;
>>>>        ^
>>>>~/gcc/trunk/inst/lib/gcc/x86_64-pc-linux-gnu/12.0.0/../../../../include/c++/12.0.0/ostream:727:33: note: passing argument to parameter here
>>>>  __rval_streamable(ios_base* = (_Tp*)nullptr);
>>>>                              ^
>>>>1 error generated.
>>
>>It looks like the failed conversion with the default argument is not
>>in the immediate context, so is an error not a substitution failure.
>>Clang is probably right, so I'll change it.
>>
>>The reason I did it that way was to save instantiating
>>std::is_convertible but also because it seemed like an easy way to
>>avoid confusing diagnostics that say:
>>
>>error: forming pointer to reference type 'std::basic_ostream<char>&'
>>
>>for overload resolution failures for operator<< (because those
>>diagnostics are already hundreds of lines long and confusing enough
>>already).
>>
>>This seems to work:
>>
>>--- a/libstdc++-v3/include/std/ostream
>>+++ b/libstdc++-v3/include/std/ostream
>>@@ -722,9 +722,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>    __rval_streamable();
>>#else
>>  template<typename _Tp,
>>-          typename = _Require<__not_<__is_one_of<_Tp, _Tp&, ios_base>>>>
>>+          typename = _Require<__not_<is_same<_Tp, ios_base>>,
>>+                              is_convertible<_Tp*, ios_base*>>>
>>    _Tp&
>>-    __rval_streamable(ios_base* = (_Tp*)nullptr);
>>+    __rval_streamable();
>>#endif
>>
>>  // SFINAE helper to check constraints for operator<<(Ostream&&, const T&).
>>
>>
>>I'll finish testing that.
>
>Actually, if the function parameter can't be used for the convertible
>check then there's no benefit to that function at all. It's simpler to
>just put all the constraints directly on the alias template that also
>checks the operator<< expression:
>
>  // SFINAE helper to check constraints for operator<<(Ostream&&, const T&).
>  // If the constraints are satisfied, it is an alias for Ostream&&.
>#if __cpp_lib_concepts
>  // Use concepts if possible because they're cheaper to evaluate.
>  template<typename _Os, typename _Tp>
>    requires (!is_same_v<_Os, ios_base>)
>      && (!is_lvalue_reference_v<_Os>)
>      && requires (_Os* __os, ios_base* __b, const _Tp& __t) {
>	  __b = __os;
>	  *__os << __t;
>      }
>#else
>  template<typename _Os, typename _Tp,
>	   typename = _Require<__not_<is_same<_Os, ios_base>>,
>			       is_convertible<_Os*, ios_base*>>,
>	   typename
>	     = decltype(std::declval<_Os&>() << std::declval<const _Tp&>())>
>#endif
>    using __rvalue_stream_insertion_t = _Os&&;

Here's what I've pushed to trunk, and will push to gcc-11 after
testing it on the branch.

Tested x86_64-linux and powerpc64le-linux.



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

commit a87ceadf17b4a899f3e74e2da8b6b209461d2742
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu May 6 19:14:42 2021

    libstdc++: Fix constraints for rvalue stream insertion/extraction
    
    The __rval_streamable() function was an attempt to test for
    convertibility cheaply and without confusing diagnostics. It doesn't
    work with Clang though, and is probably ill-formed.
    
    Replace that helper function with a check for derivation from ios_base,
    and use that in the alias templates __rvalue_stream_insertion_t and
    __rvalue_stream_extraction_t. Use concepts for the constraints when
    available.
    
    libstdc++-v3/ChangeLog:
    
            * include/std/istream (__rvalue_stream_extraction_t): Replace
            use of __rval_streamable.
            * include/std/ostream (__rvalue_stream_insertion_t): Likewise.
            (__rval_streamable): Remove.
            (_Require_derived_from_ios_base, __derived_from_ios_base): New
            helper for checking constraints.
            * testsuite/27_io/basic_istream/extractors_other/char/4.cc: Fix
            reference to the wrong subclause of the standard.
            * testsuite/27_io/basic_istream/extractors_other/wchar_t/4.cc:
            Likewise.
            * testsuite/27_io/basic_ostream/inserters_other/char/6.cc:
            Likewise.
            * testsuite/27_io/basic_ostream/inserters_other/wchar_t/6.cc:
            Likewise.
            * testsuite/27_io/basic_ostream/inserters_other/char/99692.cc:
            New test.
            * testsuite/27_io/filesystem/path/io/dr2989.cc: Adjust pruned
            errors.

diff --git a/libstdc++-v3/include/std/istream b/libstdc++-v3/include/std/istream
index ea34cce6298..5ad60dbd709 100644
--- a/libstdc++-v3/include/std/istream
+++ b/libstdc++-v3/include/std/istream
@@ -958,12 +958,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // 2328. Rvalue stream extraction should use perfect forwarding
   // 1203. More useful rvalue stream insertion
 
-  // SFINAE helper to check constraints for operator>>(Istream&&, T&&).
-  // If the constraints are satisfied, it is an alias for Istream&&.
-  template<typename _Is, typename _Tp,
-	   typename = decltype(std::__rval_streamable<_Is>()
-				 >> std::declval<_Tp>())>
+#if __cpp_lib_concepts
+  template<typename _Is, typename _Tp>
+    requires __derived_from_ios_base<_Is>
+      && requires (_Is& __is, _Tp&& __t) { __is >> std::forward<_Tp>(__t); }
     using __rvalue_stream_extraction_t = _Is&&;
+#else
+  template<typename _Is, typename _Tp,
+	   typename = _Require_derived_from_ios_base<_Is>,
+	   typename = decltype(std::declval<_Is&>() >> std::declval<_Tp>())>
+    using __rvalue_stream_extraction_t = _Is&&;
+#endif
 
   /**
    *  @brief  Generic extractor for rvalue stream
diff --git a/libstdc++-v3/include/std/ostream b/libstdc++-v3/include/std/ostream
index fdd2a87665c..981697324c9 100644
--- a/libstdc++-v3/include/std/ostream
+++ b/libstdc++-v3/include/std/ostream
@@ -708,31 +708,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   // _GLIBCXX_RESOLVE_LIB_DEFECTS
   // 1203. More useful rvalue stream insertion
 
-  // SFINAE helper to check constraints for operator<<(Ostream&&, const T&).
-  // If Ostream is publicly and unambiguously derived from ios_base, then
-  // __rval_streamable<Ostream>() is equivalent to declval<Ostream&>().
-  // Otherwise, it results in a substitution failure. Specifically, it will
-  // fail if Ostream is an lvalue reference or the same type as ios_base.
-  // Use concepts if possible because they're cheaper to evaluate.
 #if __cpp_lib_concepts
+  // Use concepts if possible because they're cheaper to evaluate.
   template<typename _Tp>
-    requires (!is_same_v<_Tp, ios_base>)
-      && requires (_Tp* __t, ios_base* __b) { __b = __t; }
-    _Tp&
-    __rval_streamable();
-#else
-  template<typename _Tp,
-	   typename = _Require<__not_<__is_one_of<_Tp, _Tp&, ios_base>>>>
-    _Tp&
-    __rval_streamable(ios_base* = (_Tp*)nullptr);
-#endif
+    concept __derived_from_ios_base = is_class_v<_Tp>
+      && (!is_same_v<_Tp, ios_base>)
+      && requires (_Tp* __t, ios_base* __b) { __b = __t; };
 
-  // SFINAE helper to check constraints for operator<<(Ostream&&, const T&).
-  // If the constraints are satisfied, it is an alias for Ostream&&.
-  template<typename _Os, typename _Tp,
-	   typename = decltype(std::__rval_streamable<_Os>()
-				 << std::declval<const _Tp&>())>
+  template<typename _Os, typename _Tp>
+    requires __derived_from_ios_base<_Os>
+      && requires (_Os& __os, const _Tp& __t) { __os << __t; }
     using __rvalue_stream_insertion_t = _Os&&;
+#else
+  template<typename _Tp>
+    using _Require_derived_from_ios_base
+      = _Require<is_class<_Tp>, __not_<is_same<_Tp, ios_base>>,
+		 is_convertible<typename add_pointer<_Tp>::type, ios_base*>>;
+
+  template<typename _Os, typename _Tp,
+	   typename = _Require_derived_from_ios_base<_Os>,
+	   typename
+	     = decltype(std::declval<_Os&>() << std::declval<const _Tp&>())>
+    using __rvalue_stream_insertion_t = _Os&&;
+#endif
 
   /**
    *  @brief  Generic inserter for rvalue stream
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_other/char/4.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_other/char/4.cc
index 9f1e293474f..94e41a17e82 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_other/char/4.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_other/char/4.cc
@@ -17,7 +17,7 @@
 // with this library; see the file COPYING3.  If not see
 // <http://www.gnu.org/licenses/>.
 
-// 27.6.2.5.3 basic_ostream manipulator inserters
+// C++11 27.7.2.6 Rvalue stream extraction [istream.rvalue]
 
 #include <sstream>
 
diff --git a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_other/wchar_t/4.cc b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_other/wchar_t/4.cc
index fc7f5505bf4..b182be7ee3d 100644
--- a/libstdc++-v3/testsuite/27_io/basic_istream/extractors_other/wchar_t/4.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_istream/extractors_other/wchar_t/4.cc
@@ -17,7 +17,7 @@
 // with this library; see the file COPYING3.  If not see
 // <http://www.gnu.org/licenses/>.
 
-// 27.6.2.5.3 basic_ostream manipulator inserters
+// C++11 27.7.2.6 Rvalue stream extraction [istream.rvalue]
 
 #include <sstream>
 
diff --git a/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/char/6.cc b/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/char/6.cc
index 4801afcba6c..3a748cd630b 100644
--- a/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/char/6.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/char/6.cc
@@ -17,7 +17,7 @@
 // with this library; see the file COPYING3.  If not see
 // <http://www.gnu.org/licenses/>.
 
-// 27.6.2.5.3 basic_ostream manipulator inserters
+// C++11 27.7.3.9 Rvalue stream insertion [ostream.rvalue]
 
 #include <sstream>
 
diff --git a/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/char/99692.cc b/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/char/99692.cc
new file mode 100644
index 00000000000..e41399a1bf1
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/char/99692.cc
@@ -0,0 +1,34 @@
+// { dg-do compile { target c++11 } }
+
+#include <ostream>
+
+struct CustomStream : std::ostream {};
+
+namespace N {
+    class A{};
+}
+
+std::ostream& operator<<(std::ostream& s, const N::A&)
+{
+    return s;
+}
+
+CustomStream&& operator<<(CustomStream&& s, const N::A& v)
+{
+    static_cast<std::ostream&>(s) << v;
+    return std::move(s);
+}
+
+void test_pr99692()
+{
+  // PR libstdc++/99692
+    CustomStream() << N::A{};
+}
+
+int test_shift_ios_enum()
+{
+  // https://gcc.gnu.org/pipermail/libstdc++/2021-May/052507.html
+  int i = 1 << std::ios::erase_event;
+
+  return i;
+}
diff --git a/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/wchar_t/6.cc b/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/wchar_t/6.cc
index 3efeb804b00..8fc0694dddc 100644
--- a/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/wchar_t/6.cc
+++ b/libstdc++-v3/testsuite/27_io/basic_ostream/inserters_other/wchar_t/6.cc
@@ -17,7 +17,7 @@
 // with this library; see the file COPYING3.  If not see
 // <http://www.gnu.org/licenses/>.
 
-// 27.6.2.5.3 basic_ostream manipulator inserters
+// C++11 27.7.3.9 Rvalue stream insertion [ostream.rvalue]
 
 #include <sstream>
 
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/io/dr2989.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/io/dr2989.cc
index 609f1c32a0e..c5cda776477 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/path/io/dr2989.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/path/io/dr2989.cc
@@ -33,4 +33,3 @@ void foo(std::iostream& s) {
   s >> p; // { dg-error "no match" }
 }
 // { dg-prune-output "no type .*enable_if" }
-// { dg-prune-output "no matching function for call to '__rval_streamable" }