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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

SGTM, but I like to have a quick look after the CI passes. Feel free to ping me on Discord when the CI is green.



================
Comment at: libcxx/include/ostream:1103
+
+#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
+template <class _Traits>
----------------
philnik wrote:
> Mordante wrote:
> > Next time for reviewing it would be easier to use the same order as the paper.
> I first put it in the same order as in the paper, but that would have meant 3x the amount preprocessor directives, which would be a lot less readable in the long term IMO.
That might be true, I can only say that when I would see that version. But at least the synopsis was a breeze to review :-)


================
Comment at: libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp:21-22
+  std::ostringstream s;
+  std::wostringstream sw;
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  s << wchar_t();                      // expected-error {{overload resolution selected deleted operator '<<'}}
----------------
I expect that is cause of the CI failures, but I guess you need to use more guards since `sw` is used unguarded below.


================
Comment at: libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp:24
+  s << wchar_t();                      // expected-error {{overload resolution selected deleted operator '<<'}}
+  s << std::declval<const wchar_t*>(); // expected-error {{overload resolution selected deleted operator '<<'}}
+#endif
----------------
Mainly curious, but will `std::declval<wchar_t*>();` be valid? I expect it will.


================
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
----------------
philnik wrote:
> Mordante wrote:
> > For clarity I would prefer and similar changes to other tests.
> I think explicitly naming the types makes it a lot clearer. I have to think a lot harder to know what `u8`, `u`, `U` and `L` mean (I actually had to look these up).
For me it's exactly the other way around, for these types I have to look at a lot of "noise" to see the type. But I don't feel this is wrong so feel free to keep it as-is unless other reviews also prefer `L` and friends.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129195