[libcxx-commits] [PATCH] D129195: [libc++] Implement P1423R3 (char8_t backward compatibility remediation)

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I like to see what happens in the CI. It's a bit curious no existing tests seem to be broken.



================
Comment at: libcxx/docs/Status/Cxx20Papers.csv:118
 "`P1361R2 <https://wg21.link/P1361R2>`__","LWG","Integration of chrono with text formatting","Cologne","",""
-"`P1423R3 <https://wg21.link/P1423R3>`__","LWG","char8_t backward compatibility remediation","Cologne","|In Progress|",""
+"`P1423R3 <https://wg21.link/P1423R3>`__","LWG","char8_t backward compatibility remediation","Cologne","|Complete|",""
 "`P1424R1 <https://wg21.link/P1424R1>`__","LWG","'constexpr' feature macro concerns","Cologne","Superseded by `P1902 <https://wg21.link/P1902>`__",""
----------------
Make sure to set the proper version number before landing.


================
Comment at: libcxx/include/ostream:1101
 
+#if _LIBCPP_STD_VER > 17
+
----------------
Please update the synopsis.


================
Comment at: libcxx/include/ostream:1103
+
+#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
+template <class _Traits>
----------------
Next time for reviewing it would be easier to use the same order as the paper.


================
Comment at: libcxx/include/ostream:1135
+template <class _Traits>
+basic_ostream<char, _Traits>& operator<<(basic_ostream<char, _Traits>&, const char8_t*) = delete;
+
----------------
This `char8_t` needs to be guarded.


================
Comment at: libcxx/include/ostream:1143
+
+template <class _Traits>
+basic_ostream<wchar_t, _Traits>& operator<<(basic_ostream<wchar_t, _Traits>&, const char16_t*) = delete;
----------------
These `wchar_t` need to be guarded.


================
Comment at: libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp:8
+//===----------------------------------------------------------------------===//
+
+// <ostream>
----------------
This should be restricted to version 20 and newer.


================
Comment at: libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp:21-22
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  s << wchar_t();                      // expected-error {{overload resolution selected deleted operator '<<'}}
+  s << std::declval<const wchar_t*>(); // expected-error {{overload resolution selected deleted operator '<<'}}
+#endif
----------------
For clarity I would prefer and similar changes to other tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129195/new/

https://reviews.llvm.org/D129195