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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM



================
Comment at: libcxx/include/filesystem:1131
+  generic_string(const _Allocator& __a = _Allocator()) const {
+    return string<_ECharT, _Traits, _Allocator>(__a);
+  }
----------------
mstorsjo wrote:
> 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.
Ok. And thank you for the explanation.


================
Comment at: libcxx/include/filesystem:1139
+#endif /* !_LIBCPP_HAS_NO_LOCALIZATION */
+#else /* !_WIN32 */
+
----------------
mstorsjo wrote:
> 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 */`.
Yeah that's not very clear sometimes how it should be read.
Personally I love the MS STL style with "^^^^ if-condition vvvvv negated condition".


================
Comment at: libcxx/src/filesystem/operations.cpp:176
     case PS_InRootDir:
-      return "/";
+      if (RawEntry[0] == '\\')
+        return PS("\\");
----------------
mstorsjo wrote:
> 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.
Ok.


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

https://reviews.llvm.org/D91135