[libcxx-commits] [PATCH] D103413: [libc++][format] Implement Unicode support.

Thu Sep 23 10:25:02 PDT 2021

Mordante marked 8 inline comments as done.
Mordante added subscribers: DanielMcIntosh-IBM, daltenty.
Mordante added a comment.

Thanks for the review. I'll update the patch after you land D110338 <https://reviews.llvm.org/D110338>.

================
Comment at: libcxx/benchmarks/CMakeLists.txt:148-149
           LINK_FLAGS "${BENCHMARK_TEST_LIBCXX_LINK_FLAGS}"
-          CXX_STANDARD 17
-          CXX_STANDARD_REQUIRED YES
+          CXX_STANDARD 20
+          CXX_STANDARD_REQUIRED NO
           CXX_EXTENSIONS NO)
----------------
ldionne wrote:
> Can we make those changes as part of another review? Also, let's set `CXX_STANDARD_REQUIRED YES` instead -- all supported compilers should support C++20 now.
See D110338.

================
Comment at: libcxx/benchmarks/std_format_spec_string_unicode.bench.cpp:7
+
+// TODO FMT Remove once libc++ requires C++20 support.
+#if __cplusplus > 201703L && !defined(_LIBCPP_HAS_NO_UNICODE)
----------------
ldionne wrote:
> I don't understand this `TODO`. We do require C++20 support.
This `TODO` was written at a time we didn't require it yet. Hence the `TODO` to remind me. I remove the test and the comment.

================
Comment at: libcxx/benchmarks/std_format_spec_string_unicode.bench.cpp:17-19
+#ifdef _LIBCPP_HAS_NO_UNICODE
+#error The benchmark requires Unicode support enabled.
+#endif
----------------
ldionne wrote:
> I'm not sure this is useful -- if the library doesn't support unicode, something in the benchmark is going to fail anyway. It would be nice to remove as many libc++ specific things from the benchmark as possible.
Good catch this is already tested at the start of the file. This benchmark is really intended to test some libcxx implementation details. For example the `__column_width_3` function has several different ways to implement it. This had a performance impact. If the benchmarks should be usable for other compilers/standard libraries I would rather add some guards in this test.

================
Comment at: libcxx/benchmarks/std_format_spec_string_unicode.bench.cpp:21
+
+// Always enable asserts since they are used compile-time not run-time.
+#ifdef NDEBUG
----------------
ldionne wrote:
> The benchmarks should be compiled without assertions anyway -- it seems pretty hacky to undefine this here, no?
I agree I think I used the benchmarks for testing originally. So I removed this hack and the validation.

================
Comment at: libcxx/benchmarks/std_format_spec_string_unicode.bench.cpp:84
+  static_assert(
+      sizeof(
+          "Victor jagt zwölf Boxkämpfer quer über den großen Sylter Deich") ==
----------------
ldionne wrote:
> This line break is really odd.
My clang-format settings are still at 80 columns to avoid merge conflicts in this series. I'll fix this manually.

================
Comment at: libcxx/include/__format/parser_std_format_spec.h:758
+
+/**
+ * Unicode column width estimates.
----------------
ldionne wrote:
> Just a general comment -- we generally try to use C++ style comments (`//`) elsewhere in the library. I don't have the heart to ask you to change them cause it's such a nitpick, but please consider using that style going forward for consistency.
I'll try to keep it in mind. After we switched to clang-fomrat-13 I still want to find some time to reformat the code using our new settings. Then I probably will look at fixing these comments.

================
Comment at: libcxx/include/__format/parser_std_format_spec.h:876
+ */
+_LIBCPP_HIDE_FROM_ABI constexpr int __column_width(uint32_t __c) noexcept {
+  if (__c < 0x1'0000)
----------------
ldionne wrote:
> This needs to be `inline` since it's not a template and it's in a header. This certainly applies elsewhere too, please take a look. Otherwise, those are ODR violations waiting to happen (arguably those ODR violations are benign, but still).
I omitted the `inline` since `constexpr` functions are implicitly inline, guarding against ODR violations.
http://eel.is/c++draft/dcl.constexpr#1

```
A function or static data member declared with the constexpr or consteval specifier is implicitly an inline function or variable ([dcl.inline]).
```
Did I misinterpret this rule?
Do you still prefer the `inline`?

================
Comment at: libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_string_unicode.pass.cpp:1
+//===----------------------------------------------------------------------===//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
ldionne wrote:
> I would suggest splitting this into two files and using the `libcpp-has-no-unicode` Lit feature to enable/disable them. That sounds a bit cleaner since essentially the whole test is different depending on whether unicode is supported. Your call, this is not a blocking comment.
I like your suggestion it indeed sounds like a cleaner solution.

================
Comment at: libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_string_unicode.pass.cpp:50
+
+#ifndef _LIBCPP_HAS_NO_UNICODE
+template <class CharT>
----------------
ldionne wrote:
> It just occurred to me that I am confused by the existence of both `_LIBCPP_HAS_NO_UNICODE` and `_LIBCPP_HAS_NO_UNICODE_CHARS` -- can you shed some light on this if it's fresh in your mind?
`_LIBCPP_HAS_NO_UNICODE` is the switch I added so the user can disable Unicode output. It's introduced in libc++13 using a CMake option. (If needed we can look whether this define can/should be renamed.)

`_LIBCPP_HAS_NO_UNICODE_CHARS` is not fresh in my mind, but grep does miracles ;-)
`_LIBCPP_HAS_NO_UNICODE_CHARS` is enabled in `__config` when the IBM compiler is used.
It was added by Howard in 2013, I've no idea whether that's still needed.
@DanielMcIntosh-IBM, @daltenty do you know whether the IBM compiler has proper support for `char16_t` and `char32_t`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103413