[libcxx-commits] [PATCH] D100005: [libc++] Use the default initializer for char_type in std::num_get::do_get.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/locale:1072
     char_type __atoms[26];
-    char_type __thousands_sep = 0;
+    char_type __thousands_sep = '\x00';
     string __grouping;
----------------
curdeius wrote:
> Wouldn't this be more generic?
> 
> I doubt that `char_type` must be constructible from `char` (at least, no more than from `int`).
> And I guess that a type that is not default-initializable would fail in other places anyway.
I would like to see some evidence that `CharT` is permitted to be //anything// other than `char` or `wchar_t`. If it is, then there ought to be a line in the standard that permits it, and there ought to be some libc++ tests that test it.

I went through my local `libcxx/include/locale` and grepped for `TEMPLATE_VIS`. At the top of each of these templates, I added the line
```
static_assert(is_same<_CharT, char>::value || is_same<_CharT, wchar_t>::value, "");
```
Then I ran all the tests in `libcxx/test/*/localization/`. They all passed.

So I'd be favorably inclined toward a patch that just added those static_asserts (putting the burden of proof on some user to explain why class-typed `CharT` should be permitted at all). If this current PR is intended to formally support class-typed `CharT` in <locale>, then I would ask for testing at the level of our existing tests for `char` and `wchar_t`.

//Also//, if you want to support types other than `char` and `wchar_t`, I think you should have a "concept" for what types you're planning to support. The obvious extension would be "character types," or even "integral types"; but you want to go further, into class types. But not //all// class types, obviously! You can't have a `std::num_get<std::mutex>`! So, what operations/affordances do you think we actually need out of a "character-like type"? Personally, I would have put "it has a zero value" //very// high on that list. What if someone gives us a move-only `CharT` type? or a `CharT` with an overloaded `operator&` or `operator,`? Where's the line drawn, and can you write it down so we all agree on it?

This PR is basically trivial — just changing `0` to `char_type()` in one place — //but// it is also a readability regression, in the sense that `x = 0` is clearly //better code// than `x = char_type()`. All else being equal, we'd obviously prefer to write `0`. So if we're going to make this trivial change, we should have a technical reason to do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100005