[libcxx-commits] [PATCH] D150031: [libc++][format] Adds a UTF transcoder.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added a comment.

Thanks for the reviews!



================
Comment at: libcxx/include/print:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
ldionne wrote:
> Are we going to have a lot of stuff inside `<print>`? If so, it would probably make sense to introduce this utility under `__print/encode.h` instead.
No print is small, 2 more overloads and it's complete. I want to move the encoding part to a new header once P2728 is voted in. Then the code becomes part of that header.


================
Comment at: libcxx/include/print:44
+template <class _Tp>
+concept __utf16_code_unit = same_as<_Tp, char16_t> _LIBCPP_IF_WIDE_CHARACTERS(|| same_as<_Tp, wchar_t>);
+template <class _Tp>
----------------
ldionne wrote:
> Can we use `#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS` instead? I'd like to reduce usage of `_LIBCPP_IF_WIDE_CHARACTERS` and eventually remove it since it's a really unusual macro.
To be honest I feel the macro is easier to read. I don't feel too strongly about it so I change it.


================
Comment at: libcxx/test/libcxx/input.output/iostream.format/print.fun/transcoding.pass.cpp:9-10
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// TODO FMT Fix this test using GCC, it currently times out.
+// UNSUPPORTED: gcc-12
+
----------------
ldionne wrote:
> Does this really time out on GCC 12? What's special about this test that makes it timeout?
Actually it's the same GCC12 issue as the other format parts. This patch was written before we started to use `// UNSUPPORTED: GCC-ALWAYS_INLINE-FIXME` so I copied it from some format header. I've changed it to the `GCC-ALWAYS_INLINE-FIXME`



================
Comment at: libcxx/test/libcxx/input.output/iostream.format/print.fun/transcoding.pass.cpp:18-22
+#include <algorithm>
+#include <cassert>
+#include <print>
+#include <string_view>
+#include <array>
----------------
ldionne wrote:
> Those don't seem sorted :)
I trust on `clang-format` ;-) Fixed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150031