[libcxx-commits] [PATCH] D111265: [libc++] Add an option to disable wide character support in libc++

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/CMakeLists.txt:123
    embedded)." ON)
+option(LIBCXX_ENABLE_WIDE_CHARACTERS
+  "Whether to include support for wide characters in the library. Disabling
----------------
Mordante wrote:
> Can you add this option to `docs/BuildingLibcxx.rst`?
> 
> Is this feature worth mentioning in the release notes?
Did both, good suggestions!

As a self note, I don't really like the duplication of information between the CMake file and the Sphinx documentation for build options. They are often out-of-sync and not everything is everywhere. But I just wanted to raise awareness about the situation -- this isn't the right place to fix it.


================
Comment at: libcxx/include/regex:1073
     template <class _ForwardIterator>
-        string_type
-        __transform_primary(_ForwardIterator __f, _ForwardIterator __l, char) const;
----------------
Mordante wrote:
> I'm not fond of formatting changes in this patch. I would strongly recommend to just commit the format changes now so this patch looks cleaner.
You're right, sorry this slipped through. It should be one of the only places where there are such changes, and I'll avoid touching the formatting at all.


================
Comment at: libcxx/src/locale.cpp:4665
         lconv* lc = __libcpp_localeconv_l(loc.get());
         checked_string_to_char_convert(__decimal_point_, lc->decimal_point,
                                        loc.get());
----------------
Mordante wrote:
> I think we should validate the return value here and two lines below.
> Something like done in `moneypunct_byname<char, false>`
> ```
>     if (!checked_string_to_char_convert(__decimal_point_,
>                                         lc->mon_decimal_point,
>                                         loc.get()))
>       __decimal_point_ = base::do_decimal_point();
> ```
Thanks, honestly `<locale>` is gibberish to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111265