[libcxx-commits] [PATCH] D91135: [3/N] [libcxx] Make filesystem::path::value_type wchar_t on windows

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added inline comments.


================
Comment at: libcxx/include/filesystem:727
+#if defined(_LIBCPP_WIN32API)
+    string __utf8;
+    _Narrower()(back_inserter(__utf8), __tmp.data(),
----------------
Wouldn't it be wise to reserve at least the minimum expected output size?
It seems that there are some places that a reserve would fit well.
But that's out of scope of this patch.


================
Comment at: libcxx/include/filesystem:1131
+  generic_string(const _Allocator& __a = _Allocator()) const {
+    return string<_ECharT, _Traits, _Allocator>(__a);
+  }
----------------
Shouldn't there be a difference between string and generic_string, the former returning native format (with backslash on Windows?) and the latter always path with forward slashes?


================
Comment at: libcxx/include/filesystem:1139
+#endif /* !_LIBCPP_HAS_NO_LOCALIZATION */
+#else /* !_WIN32 */
+
----------------
The comment doesn't match the #if.


================
Comment at: libcxx/include/filesystem:1300
+      typename enable_if<is_same<_CharT, value_type>::value &&
                              is_same<_Traits, char_traits<char> >::value,
                          basic_ostream<_CharT, _Traits>&>::type
----------------
Hmm?


================
Comment at: libcxx/include/filesystem:1310
+      typename enable_if<!is_same<_CharT, value_type>::value ||
                              !is_same<_Traits, char_traits<char> >::value,
                          basic_ostream<_CharT, _Traits>&>::type
----------------
Ditto.


================
Comment at: libcxx/src/filesystem/operations.cpp:176
     case PS_InRootDir:
-      return "/";
+      if (RawEntry[0] == '\\')
+        return PS("\\");
----------------
Why does it depend on RawEntry and not on OS only?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91135