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

[Louis Dionne via Phabricator]

ldionne added a comment.

In D55308#1323439 <https://reviews.llvm.org/D55308#1323439>, @mclow.lists wrote:

> I would not; consider the test `algorithm.version.pass.cpp`, which should check for six different flags. Do you want that test to fail until libc++ implements all six of them? I do not. I want the tests to check for all the flags that libc++ has defined, and make sure that they are (a) defined, and (b) have a sane value. Things that we haven't implemented yet should not cause test failures.


I disagree here (in an ideal world). Otherwise, what we have is not a C++ Standard conformance test suite, it's a libc++ test suite only. But okay, that's part of a larger discussion anyway.

> 
> 
>> But it would be nice to extract this check into a separate utility header to avoid duplication.
> 
> I don't think that's a good idea; that header would quickly grow into all the tests for all the feature flags.
>  But I'm willing to discuss that.

That would be one header per feature-test macro. Maybe that's too much, but I dislike the replication. Leave it as-is, we can always change later when we define more feature test macros if it makes sense then.



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


================
Comment at: include/__string:484
+    }
+    return 0;
+}
----------------
`nullptr`?


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


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

https://reviews.llvm.org/D55308