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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added a comment.

It looks pretty good for me, just minor comments.
And please put the relevant papers in the patch description, so that they appear in the commit later.



================
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 ||
----------------
Unnecessary comment? You should maybe mark [[ https://wg21.link/P1483 | P1483]] as In Progress.


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


================
Comment at: libcxx/include/filesystem:1251-1255
+      // Proposed in P1423
+      is_same<typename __is_pathable<_InputIt>::__char_type, char8_t>::value ||
+#endif
       is_same<typename __is_pathable<_InputIt>::__char_type, char>::value,
       "u8path(Iter, Iter) requires Iter have a value_type of type 'char'");
----------------
Ditto. Comment about P1423 and message.


================
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)}
 
----------------
Shouldn't you guard it on `__cpp_char8_t` instead of using `dummy` below?


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

https://reviews.llvm.org/D90222