[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");
----------------
Mordante wrote:
> tahonermann wrote:
> > 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.
> Thanks for the additional link. The assumption `_CharT('\t')` that works correctly is hard-coded in several places in libc++ (not limited to the format library). I think it's good to address this, but I prefer to do it in a separate review. I really want to look at how we're going to do that properly and properly test it.
> 
> (I expect we need a CI job where we use different encodings for `char` and `wchar_t`.)
Doing so separately makes sense to me. And I agree that a CI job is needed once a solution is in place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134036