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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/include/filesystem:1131
+  generic_string(const _Allocator& __a = _Allocator()) const {
+    return string<_ECharT, _Traits, _Allocator>(__a);
+  }
----------------
curdeius wrote:
> 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?
Yes, there's a difference; that aspect is corrected later in patch [24/N] in D91181. (I understand that it'd be cleaner to review if all changed code lands in the right form directly the first time it's touched - but that'd make the first patches even more unwieldy - so here I try to primarily focus on getting the wchar_t aspect right.)

And in general, string() and native() returns the string with whatever separator form the string originally contained, it can be any combination of forward and backslashes, while generic_string() indeed returns a version with forward slashes only.


================
Comment at: libcxx/include/filesystem:1139
+#endif /* !_LIBCPP_HAS_NO_LOCALIZATION */
+#else /* !_WIN32 */
+
----------------
curdeius wrote:
> The comment doesn't match the #if.
Right, I guess I should make it `_LIBCPP_WIN32API` instead, and the `!` is confusing. I meant it in the form `#else /* condition *` as it could be `#elseif condition` i.e. describing the condition for the code following the `#else`, not the condition for the block above, but I guess that's not common practice. I'll change it to just plain `#else /* _LIBCPP_WIN32API */`.


================
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
----------------
curdeius wrote:
> Hmm?
Right, this was a generalization from before I added the more windows specific bits, trying to move things to use explicitly `value_type` and `string_type` instead of hardcoding the types - but here it's a bit out of place, so I can drop these two changes from the patch.


================
Comment at: libcxx/src/filesystem/operations.cpp:176
     case PS_InRootDir:
-      return "/";
+      if (RawEntry[0] == '\\')
+        return PS("\\");
----------------
curdeius wrote:
> Why does it depend on RawEntry and not on OS only?
Because when iterating over the path name, we want to return the root dir with the slash in the same form as in the actual path itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91135