[PATCH] D55308: Implement the second part of P0482 (char8_t)

[Marshall Clow via Phabricator]

mclow.lists marked an inline comment as done.
mclow.lists added inline comments.


================
Comment at: include/string:4173
 
+#if _LIBCPP_STD_VER > 17 && defined(__cpp_char8_t)
+typedef basic_string<char8_t> u8string;
----------------
ldionne wrote:
> mclow.lists wrote:
> > mclow.lists wrote:
> > > ldionne wrote:
> > > > mclow.lists wrote:
> > > > > ldionne wrote:
> > > > > > I hate feature test macros already. This should just be `#if _LIBCPP_STD_VER > 17`... Can't we take the slightly more aggressive stance of "if you're trying to use libc++ as a c++20 library but your compiler does not support c++20, you're out of luck"?
> > > > > > I hate feature test macros already.
> > > > > Agreed.
> > > > > 
> > > > > > Can't we take the slightly more aggressive stance of "if you're trying to use libc++ as a c++20 library but your compiler does not support c++20, you're out of luck"
> > > > > 
> > > > > We can't do that, because (for example) LLVM 7.0 claims to support C++20, but doesn't support `char8_t`.
> > > > > 
> > > > > In general, I agree with you, but this feature (`char8_t`) requires a compiler feature, or it doesn't work.
> > > > > We can't do that, because (for example) LLVM 7.0 claims to support C++20, but doesn't support char8_t.
> > > > 
> > > > Isn't that claim wrong, then? Also, surely you can't claim you implement C++20 at this point since it hasn't been voted?
> > > > 
> > > > I'm not trying to be rhetorical -- I'd _really_ like to avoid having to guard everything with a feature test macro because that will make the library much more complex and I'm sure it will introduce some tricky bugs. Just supporting many compilers and many C++ Standard versions is already difficult -- supporting partial implementations of a standard scares me a lot.
> > > I agree; we don't want to do that; and, in general, we won't need to do so.
> > > 
> > > BUT, in this case, we need a particular bit of functionality from the compiler, and the way to see if the compiler has implemented it is via a feature-test macro.  In other places, we use `__has_builtin` or other ways of querying the compiler. This is the first place (that I know of) that we're using a feature-test macro to do so.
> > > 
> > Would you feel better if I defined `_LIBCPP_HAS_CHAR8_T` in `<__config>` and used that throughout?
> > 
> > It would look like this:
> > ```
> > #if _LIBCPP_STD_VER > 17 && defined(__cpp_char8_t)
> > #define _LIBCPP_HAS_CHAR8_T
> > #endif
> > ```
> > 
> Ah, I see. No, I think using `__cpp_char8_t` is OK in that case -- I wasn't seeing it that way.
Instead I turned it around:

```
#if _LIBCPP_STD_VER <= 17 || !defined(__cpp_char8_t)
#define _LIBCPP_HAS_NO_CHAR8_T
#endif
```

That way someone can #define _LIBCPP_HAS_NO_CHAR8_T and it will all go away.


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

https://reviews.llvm.org/D55308