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

[Tom Honermann via Phabricator via libcxx-commits]

tahonermann added a comment.

Sorry for the long delay in responding.

It looks like the changes need to be rebased; `libcxx/docs/UsingLibcxx.rst` appears to be removing unrelated content.

I see there is an existing `wclog.sh.cpp` test for which "wide-mode" and "imbue" variants aren't added by this change. I'm having a hard time convincing myself that I care though; you're call whether you want to add those for completeness or call this good enough.

I think this looks good.  The only thing I care about addressing before I accept the changes is the possibly unintended changes to `libcxx/docs/UsingLibcxx.rst`.



================
Comment at: libcxx/src/std_stream.h:327
+static bool __do_fputc(char __c, FILE* __fp) {
+    if (fwrite(&__c, sizeof(__c), 1, __fp) != 1)
+        return false;
----------------
mstorsjo wrote:
> tahonermann wrote:
> > 
> Hmm, I primarily use `fwrite` here since that's what was used before anyway - if we'd have used `fputc` for that before, I would have gone with that here.
> 
> While this is called when we write one char at a time, we also have a separate special case - if `__always_noconv_` is set, we `fwrite` a whole array of chars too.
> 
> While this might avoid triggering asserts in the CRT, it still mostly outputs gibberish in that case. So I'm not sure if I'd comment this saying that this is explicitly done this way to make that case "work" somehow, since it doesn't quite work anyway.
Ok. Hmm. My desire for a comment here was driven by the presence of the comment in the `wchar_t` overload below; the comment there implies that `fputc()` doesn't work in this case (or that `fwrite()` wouldn't for `wchar_t` streams). Perhaps we should use `fputc()` here to explicitly opt-in to that assert in order to catch incorrect use of Unicode enabled streams.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146398