This is an archive of the discontinued LLVM Phabricator instance.

Add raw_ostream wrapper to the Stream class
ClosedPublic

Authored by teemperor on Aug 1 2018, 3:11 PM.

Details

Summary

This wrapper will allow us in the future to reuse LLVM methods from within the
Stream class.

Currently no test as this is intended to be an internal class that shouldn't have any
NFC. The test for this change will be the follow up patch that migrates LLDB's
LEB128 implementation to the one from LLVM.

This change also adds custom move/assignment methods to Stream, as LLVM
raw_ostream doesn't support these. As our internal stream has anyway no state,
we can just keep the same stream object around.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor edited the summary of this revision. (Show Details)Aug 1 2018, 3:13 PM
labath added a subscriber: labath.Aug 2 2018, 2:50 AM

Wouldn't it be even better to actually expose the llvm class via some accessor or something? This way we could slowly migrate existing code by changing it to write to stream.accessor() instead of stream ? (I am not saying to do that now, but it opens up possibilities for the future.)

I also found it hard to deduce who is forwarding to whom from the member names. Maybe rename m_forward_to to m_target or something like that?

teemperor set the repository for this revision to rLLDB LLDB.Aug 2 2018, 9:39 AM
teemperor updated this revision to Diff 158926.Aug 2 2018, 11:34 PM
teemperor added a reviewer: labath.
  • Renamed m_forward_to to m_target.
  • Added a getter to allow external code to use the raw_ostream API.
  • Renamed m_forward to m_forwarder.

@labath Thanks, fixed the problems you pointed out. I already made the API public in this patch, it's probably good for encouraging people to use LLVM's raw_ostream classes.

labath accepted this revision.Aug 3 2018, 1:57 AM

looks good to me. Thanks.

Btw, have you looked at how hard would it be to actually fix the places that are copying these streams?

This revision is now accepted and ready to land.Aug 3 2018, 1:57 AM
This revision was automatically updated to reflect the committed changes.

StreamTee is copying it, which is expected to be copyable when we copy CommandObjectResult around. And then I just added the copy-constructor as CommandObjectResult refactoring sound time-expensive.

StreamTee is copying it, which is expected to be copyable when we copy CommandObjectResult around. And then I just added the copy-constructor as CommandObjectResult refactoring sound time-expensive.

Sure, that's fine. I was just wondering what the obstacles are. Thanks.