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

[Marshall Clow via Phabricator]

mclow.lists marked 5 inline comments as done.
mclow.lists added inline comments.


================
Comment at: include/__string:404
+
+    static inline constexpr void assign(char_type& __c1, const char_type& __c2) noexcept
+        {__c1 = __c2;}
----------------
ldionne wrote:
> `_LIBCPP_INLINE_VISIBILITY` here and below.
Right.


================
Comment at: include/__string:484
+    }
+    return 0;
+}
----------------
ldionne wrote:
> `nullptr`?
Meh. Sure.
Not going to go back and fix the other `char_traits` specializations in this patch, though.


================
Comment at: include/string:4173
 
+#if _LIBCPP_STD_VER > 17 && defined(__cpp_char8_t)
+typedef basic_string<char8_t> u8string;
----------------
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.


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

https://reviews.llvm.org/D55308