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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for the update, I like the current version a lot more. Some minor comments, but in general I'm happy.



================
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:
> 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!
If this indeed avoids generating the dead code I prefer this solution.


================
Comment at: libcxx/src/std_stream.h:66
+#else
+    inline static constexpr bool __is_win32api_wide_char = false;
+#endif
----------------
I actually intended it not to be in the class. I don't mind it, but then we don't need the inline.


================
Comment at: libcxx/test/std/input.output/iostream.objects/wide.stream.objects/check-stderr.sh:1
+program=${1}
+expected_file=${2}
----------------
Can you add some comments to these test explaining what they do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146398