[PATCH] D39444: [Support] Make the default chunk size of raw_fd_ostream to 1 GiB.

Rui Ueyama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 30 19:41:24 PDT 2017


ruiu added inline comments.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:589-590
   // the latter has a size limit (66000 bytes or less, depending on heap usage).
-  bool ShouldWriteInChunks = !!::_isatty(FD) && !RunningWindows8OrGreater();
+  if (!!::_isatty(FD) && !RunningWindows8OrGreater())
+    MaxWriteSize = 32767;
 #endif
----------------
zturner wrote:
> ruiu wrote:
> > compnerd wrote:
> > > zturner wrote:
> > > > The comment here doesn't make sense to me.  It says "Writing a large size of output **to Windows console**", but then the code only limits the write size if it's **not** a TTY.  Isn't this check inverted?  It should be setting the max write size if it **is** a tty.
> > > > 
> > > > Furthermore, the documentation for `WriteConsole` doesn't say anything about Windows 8, it just says the buffer is at most 64k.  It even says that the buffer could be smaller, but how much smaller depends on heap usage.  So for all we know, it might not even be 32K.
> > > > 
> > > > Granted, you're only changing the non-Windows code path, but this just looks wrong to me, in case you're interested in fixing it.
> > > Its a conversion trick (int to boolean) via double negation.
> > Actually it is !!, so it checks if isatty && Windows7OrOlder.
> > 
> > But I agree that this is confusing. I'll just remove `!!` as it doesn't seem necessary.
> Oh, duhh..  Now I feel stupid.  I use the `!!` myself too, it just wasn't registering with me for some reason.  The whole Windows 8 thing still seems dubious to me, since there's no reference (e.g. msdn link) about why this is specific to Windows 7 and below.
> 
I think we can just remove `!RunningWindows8OrGreater()`, as limiting write size to 32 KiB is not that inefficient, and when we are writing to a console, console's speed is a bottleneck anyway. But that should be done in a separate patch.


https://reviews.llvm.org/D39444





More information about the llvm-commits mailing list