[libcxx-commits] [PATCH] D146398: [libcxx] Fix using std::wcout/wcin on Windows with streams configured in wide mode

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/src/std_stream.h:155
+          // know), let it know that it doesn't need to instantiate this below.
+          if constexpr (is_same_v<char_type, char>)
+#endif
----------------
mstorsjo wrote:
> Mordante wrote:
> > How bad would it be to generate this dead code? It current code looks a bit tricky.
> > 
> > How do you feel about adding a helper variable
> > ```
> > #if defined(_LIBCPP_WIN32API)
> > template<class CharT>
> > inline constexpr bool __is_win32api_wide_char = !is_same_v<_CharT, char>;
> > #else
> > template<class>
> > inline constexpr bool __is_win32api_wide_char = false;
> > #endif
> > ```
> > 
> > and use that? That also removes the `#ifdef`. Can you test locally whether that makes the code better readable?
> Yes, this is very much not great, but unfortunately we need to avoid generating this dead code here, unless we do much larger refactorings here. Maybe there's a better way to do it though?
> 
> We need to change the type of `__extbuf` (line 125) in order to not truncate the chars.
> 
> If we try to generate dead code for this block here, the `__cv_->in` call below fails since `__extbuf` has got the wrong type for the conversion source for the `codecvt`. `__cv_` is defined as `const codecvt<char_type, char, state_type>* __cv_;` Ideally, I'd prefer to make it `const codecvt<char_type, char_type, state_type>* __cv_;` for Windows, i.e. making it a no-op (which we wouldn't call anyway), but `codecvt` doesn't support that, it only supports combinations where the second type is `char` - it can't be `<wchar_t, wchar_t>`.
> 
> So by setting `__always_noconv_` we avoid trying to use the `codecvt` at runtime, but we need to skip actual code generation for this case here...
> 
> Thanks for the tip with `__is_win32api_wide_char`, let me try that! That could simplify this (and other cases) a fair bit.
Thanks, that suggestion with `__is_win32api_wide_char` made the change much more palatable!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146398