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

Fri Jul 19 04:38:49 PDT 2019

jhenderson added a comment.

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?

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

https://reviews.llvm.org/D64969