This implements the Grapheme clustering as required by
P1868R2 width: clarifying units of width and precision in std::format
This was omitted in the initial patch, but the paper was marked as completed. This really completes the paper.
Paths
| Differential D126971
[libc++] Implements Unicode grapheme clustering ClosedPublic Authored by Mordante on Jun 3 2022, 9:10 AM.
Details
Summary This implements the Grapheme clustering as required by This was omitted in the initial patch, but the paper was marked as completed. This really completes the paper.
Diff Detail
Unit TestsFailed Event TimelineComment Actions Fixes some CI issue. Comment Actions Use a better way to store the data. Note the code and comment needs more polish. Mordante added a parent revision: D128846: [libc++[format][NFC] Removes dead code..Jul 3 2022, 1:26 AM Comment Actions Minor cleanups and addresses @STL_MSFT's review comments while upstreaming some of these changes. Comment Actions Mostly looks good but at least one output doesn't look right which suggests an error in width estimation. Also a few nits inline.
This revision now requires changes to proceed.Jul 9 2022, 8:30 AM Comment Actions Thanks a lot for your review!
Comment Actions I have some comments. Being a total unicode newbie, this is mostly out of my depth, but the general approach looks sensible to me. I'll have a closer look at https://www.unicode.org/reports/tr29/tr29-35.html#Grapheme_Cluster_Boundaries to provide a more meaningful review, but this basically LGTM via the assumption that the test coverage would catch issues in the implementation.
Comment Actions Thanks for the reviews!
Mordante marked 2 inline comments as done. Comment ActionsAddresses review comments and updates the Unicode tables to version 14. Mordante added a child revision: D129668: [libc++] Improve updating data files..Jul 13 2022, 11:04 AM Closed by commit rG857a78c04dee: [libc++] Implements Unicode grapheme clustering (authored by Mordante). · Explain Why This revision was automatically updated to reflect the committed changes. Mordante marked an inline comment as done. Comment Actions We're seeing failures after this patch https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-arm64/b8808100903143342625/overview could you please take a look? Comment Actions
It seems one of the clang-tidy changes flags the current code. I should be able to fix this rather fast. Mordante added inline comments.
Revision Contents
Diff 434273 libcxx/benchmarks/std_format_spec_string_unicode.bench.cpplibcxx/include/CMakeLists.txt
libcxx/include/__format/extended_grapheme_cluster_table.h
libcxx/include/__format/parser_std_format_spec.h
libcxx/include/__format/unicode.h
libcxx/include/format
libcxx/include/module.modulemap
libcxx/test/libcxx/private_headers.verify.cpp
libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.h
libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.pass.cpp
libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_string_unicode.pass.cpp
libcxx/test/std/utilities/format/format.functions/format_tests.h
libcxx/utils/generate_extended_grapheme_cluster_table.py
libcxx/utils/generate_extended_grapheme_cluster_test.py
|
Looks like this include is no longer needed since you removed the only std::array usage.