[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 accepted this revision.
tahonermann added a comment.

This looks good to me. I recommend getting an approval from @Mordante or @ldionne before pushing the change though.



================
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:
> > 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.
> Ah, I see.
> 
> I could clarify that comment like this:
> ```
> // fputwc works regardless of wide/narrow mode of stdout, while
> // fwrite of wchar_t only works if the stream actually has been set
> // into wide mode.
> ```
> 
> I'd kinda rather not change the use of fwrite into fput for the existing narrow case; I'm mildly afraid of affecting behaviours (including performance) on other platforms. If we really do want to change that, it could be an entirely separate change (which can be reverted individually if needed), but I'd prefer to hold off of it at this point...
Understood. I think the updated comment is good; I'm content with this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146398