[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:795
 
+#if defined(_LIBCPP_WIN32API)
+template <class _ECharT>
----------------
ldionne wrote:
> This code is valid even outside of Windows, right? If so, I'd rather we keep it in the header unconditionally. I don't mind if it's unused outside of Windows -- I think it's better to try to remove these `#ifdef`s as much as possible.
The code added here in this patch should be valid code on any platform, yes, but it's code that assumes the source char type is `wchar_t` (which is kind of not nice to have outside of specific ifdefs), and D91137 adds more code here that calls the windows-specific `__wide_to_char` and `__char_to_wide` here, so those would need to be ifdeffed anyway (as long as we keep those declarations under an ifdef).

There's of course the benefit of having more syntax checked on non-windows platforms though, even though it's never actually referenced. With other ifdefs, it could look a bit odd, like this:
```
// Windows specific code, unused elsewhere
template <class _ECharT> struct _PathExport {
    void first_method() {
    }
#ifdef _LIBCPP_WIN32API
    void other_method() {
        // Code referencing things that don't build on other platforms, referencing helpers like __wide_to_char
    }
#endif
    void third_method() {
    }
};
// End of windows specific block
```


================
Comment at: libcxx/include/filesystem:1110-1122
+  _LIBCPP_INLINE_VISIBILITY _VSTD::string string() const {
+    return string<char>();
+  }
+  _LIBCPP_INLINE_VISIBILITY __u8_string u8string() const {
+    return string<__u8_string::value_type>();
+  }
+
----------------
ldionne wrote:
> Would it be possible to share these definitions between Windows and non-Windows to avoid code duplication?
At least `u16string()` and `u32string()` could be shared, the rest is a bit messier. On posix, `string()` and `u8string()` just return `__pn_` directly and work outside of `!_LIBCPP_HAS_NO_LOCALIZATION`, while on windows, it's `wstring()` that works without localization and `string()` and `u8string()`. And later, D91181 adds more windows specifics to `generic_*string()`. So all in all, yes, some small bits could be shared, but I'm not entirely convinced it's worth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91135