[libcxx-commits] [PATCH] D134036: [libc++][format] Implements string escaping.

[Tom Honermann via Phabricator via libcxx-commits]

tahonermann added inline comments.


================
Comment at: libcxx/include/__format/formatter_output.h:446
+  switch (__value) {
+  case _CharT('\t'):
+    __str += _LIBCPP_STATICALLY_WIDEN(_CharT, "\\t");
----------------
tahonermann wrote:
> I think these case statements should use `_LIBCPP_STATICALLY_WIDEN` or similar as there is no guarantee that the `wchar_t` value for `\t` is the same as for `char`.
Any further thoughts on this? Or is there an existing libc++ policy that requires that characters that can be represented in a single code unit in both the ordinary and wide literal encodings must have the same value? If there is, I expect that policy will have to be revisited if `-fexec-charset` is expanded to include additional encodings like IBM-1047 as was proposed in D93031.


================
Comment at: libcxx/include/__format/formatter_output.h:517-519
+        _LIBCPP_ASSERT(__result.__ill_formed_size == 1, "for UTF-16 at most two code units per code point.");
+        for (int __i = 0; __i < __result.__ill_formed_size; __result.__value >>= 16, (void)++__i)
+          __formatter::__write_escape_ill_formed_code_unit(__str, __result.__value & 0xffff);
----------------
tahonermann wrote:
> `__consume_p2286()` has a condition where a `__consume_p2286_result` value is returned with `__ill_formed_size` equal to 2 that would appear to violate the assertion here. The assertion of `1` is also at odds with the comment and the need for the following loop. I suspect this is intended to assert a value of either `1` or `2`. If so, this implies a missing test case.
The changes made address most of the concern I raised, but if `__ill_formed_size` can only be `1`, then there is no need for the `for` loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134036