[PATCH] D64969: [llvm-objdump][NFC] Make the PrettyPrinter::printInst() output buffered

[Seiya Nuta via Phabricator via llvm-commits]

seiya added a comment.

In D64969#1593271 <https://reviews.llvm.org/D64969#1593271>, @jhenderson wrote:

> In D64969#1593267 <https://reviews.llvm.org/D64969#1593267>, @seiya wrote:
>
> > In D64969#1593100 <https://reviews.llvm.org/D64969#1593100>, @jhenderson wrote:
> >
> > > I've added a few more reviewers for visibility.
> > >
> > > My initial thought was "why not just delete the "flush"" from `printInst`, but I see that it's there for a "good" reason, looking at `PrettyPrinter`. I can't help but feel that a better solution would be to fix formatted_raw_ostream so that the indentation only gets added after any already-written content. Assuming that's not viable (e.g. it's intentional behaviour), I think what you have written works. @seiya, what are your thoughts on the formatted_raw_ostream implementation? Can it be sensibly changed?
> >
> >
> > First I thought that it is not straightforward to update the position (what `FOS.getColumn()` returns) in formatted_raw_stream without flushing because it's done in its write_impl.
> >
> > However, now I realized that it's trivial: add new method `formatted_raw_ostream::updatePositionByBufferedString()` (I couldn't come up with a good name) method, which reads the buffered string and updates the position by doing `ComputePosition(getBufferStart(), GetNumBytesInBuffer());` .
> >
> > I haven't tried this, but with this method, we no longer need to add a string buffer like this current patch. What do you think on this approach?
>
>
> It's hard to envisage this without seeing a patch with it in. Could you create a separate patch to this one, with your alternative proposal in, so that I and others can compare and contrast the two options, please?


I had a look at formatted_raw_ostream, but the idea I proposed in a comment is not effective and this current patch would be better. Here's why:

formatted_raw_ostream takes a raw_ostream (`outs()` in this context) and tracks the current column/line position.  Since formatted_raw_ostream itself is buffered, it flushes and makes the wrapped stream ( `outs()`) unbuffered in the constructor, and restore its buffer settings in the destructor [1].

To prevent outs() from being flushed, I tried making formatted_raw_ostream unbuffered and keeping `outs()` buffered, however, `outs()` is buffered only if it's //not// tty (line buffering is not implemented and it is stated that //"it's' not worth the complexity"// [2]).  That is, what we need to do is implementing line buffering for stdout (this patch implements a sort of it by InstString).

I think this patch is a simple workaround for this, but working on making `outs()` line-buffered instead may be better. What do you think which way sounds better?

[1]: https://github.com/llvm/llvm-project/blob/2946cd701067404b99c39fb29dc9c74bd7193eb3/llvm/include/llvm/Support/FormattedStream.h#L83
[2]: https://github.com/llvm/llvm-project/blob/49a3ad21d6034eb20f99f228dbebcc5f65a748d8/llvm/lib/Support/raw_ostream.cpp#L773


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

https://reviews.llvm.org/D64969