[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 added inline comments.


================
Comment at: libcxx/include/locale:1072
     char_type __atoms[26];
-    char_type __thousands_sep = 0;
+    char_type __thousands_sep = '\x00';
     string __grouping;
----------------
Quuxplusone wrote:
> 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.
I retract my objection here, since this seems like a deep can of worms, and the patch //is// trivial. My current impression is that (1) libc++ is //not// required to support this use-case; (2) but it is //permitted// to support "implementation-defined character types" which are not "ordinary character types," which I agree must be meant to signify class types that behave like ordinary character types to some implementation-defined degree; and (3) according to the OP, libstdc++'s implementation-defined rules permit OP's `class Char` where libc++'s rules currently don't, but this PR gets us closer to agreeing with libstdc++. So, okay, huge mess, PR doesn't make it worse, LGTM. :)


================
Comment at: libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/char.compile.pass.cpp:62
+} // namespace std
+void f() { new std::num_get<Char>; }
----------------
I would like this to be a .pass.cpp, not a .compile.pass.cpp, because I'd like us to test that the code not only //compiles// but also //links// (e.g. it doesn't accidentally attempt to use any undefined template specializations) and //runs with the intended behavior//.

I'm surprised CI passes; I would have thought this file needed `UNSUPPORTED: libcpp-has-no-localization`. Maybe once you make it link, it will need that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100005