[libcxx-commits] [PATCH] D126971: [libc++] Implements Unicode grapheme clustering

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


================
Comment at: libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.h:73
+
+  /// The first code point all extended grapheme clusters in the input.
+  std::vector<char32_t> code_points;
----------------
"of" is missing after point


================
Comment at: libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.pass.cpp:29
+// Validates whether the number of code points in our "database" matches with
+// the number in the Unicode. The assumtion is when the number of items per
+// property matches the code points themselves also match.
----------------
assumption


================
Comment at: libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.pass.cpp:79-81
+    test(data_utf16);
+  else
+    test(data_utf32);
----------------
Maybe generate only one of `data_utf16` and `data_utf32` if the other is not used? Or are they both used?


================
Comment at: libcxx/test/std/utilities/format/format.functions/unicode.pass.cpp:55
+  //*** 1-byte code points ***
+  check.template operator()<"{:*^3}">(SV("* *"), SV(" "));
+  check.template operator()<"{:*^3}">(SV("*~*"), SV("~"));
----------------
The `template operator()<` part make the tests a bit difficult to parse. I would recommend passing the format string as a normal argument and not as a template parameter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126971