[libcxx-commits] [PATCH] D90222: [1/N] [libcxx] Implement c++2a char8_t input/output of std::filesystem::path

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/include/filesystem:1236
+#ifndef _LIBCPP_NO_HAS_CHAR8_T
+      // Proposed in P1423
+      is_same<typename __is_pathable<_Source>::__char_type, char8_t>::value ||
----------------
curdeius wrote:
> Unnecessary comment? You should maybe mark [[ https://wg21.link/P1483 | P1483]] as In Progress.
Sure


================
Comment at: libcxx/include/filesystem:1241
       "u8path(Source const&) requires Source have a character type of type "
       "'char'");
   return path(__s);
----------------
curdeius wrote:
> You should update the message too.
Oh, good catch, thanks!


================
Comment at: libcxx/test/support/filesystem_test_helper.h:433
 
-#define MKSTR(Str) {Str, TEST_CONCAT(L, Str), TEST_CONCAT(u, Str), TEST_CONCAT(U, Str)}
+#define MKSTR(Str) {Str, TEST_CONCAT(L, Str), TEST_CONCAT(u8, Str), TEST_CONCAT(u, Str), TEST_CONCAT(U, Str)}
 
----------------
curdeius wrote:
> Shouldn't you guard it on `__cpp_char8_t` instead of using `dummy` below?
Yeah, it's possible, with something like this:
```
#if TEST_STD_VER > 17 && defined(__cpp_char8_t)
#define CHAR8_ONLY(x) x,
#else
#define CHAR8_ONLY
#endif
#define MKSTR(Str) {... CHAR8_ONLY(TEST_CONCAT(u8, Str)) TEST_CONCAT(u, Str), ...
```
The trailing comma is a bit tricky; it has to be included as part of the expansion of `CHAR8_ONLY()`, but this way it seems like it would work.

If you prefer that form, I can change it that way.


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

https://reviews.llvm.org/D90222