[Lldb-commits] [PATCH] D68546: remove FILE* usage from ReportEventState() and HandleProcessEvent()

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D68546#1699390 <https://reviews.llvm.org/D68546#1699390>, @labath wrote:

> Thanks for weighing in, Jim. My responses are inline.
>
> In D68546#1698430 <https://reviews.llvm.org/D68546#1698430>, @jingham wrote:
>
> > SBProcess::ReportEventState was introduced in r112221, and SBStream was added in r114188, so Pavel's speculation seems like a reasonable one, though that was 9 years ago...
> >
> > But in the SB API's we use SBStream in a bunch of places to be more like an SBString, something you can pass to a client and they will add some data to it, and then you call GetData to get the result.  That's a not very stream-like usage.
>
>
> Why not? I think that's fairly similar to how one uses `std::ostringstream`.
>
>   std::ostringstream stream;
>   write_some_data_to(stream); // or stream << stuff;
>   std::string data = stream.str();
>
>
> I think the main non-stream like feature of SBStream is that it behaves like std::ostringstream _by default_, but then it can be later converted to something like std::ofstream by calling `RedirectToFile`. However, I think that is something that can still be fixed, mostly..


In most of the uses of SBStream (and all the ones' I've done for the various scripts I've written), I would be very surprised if GetData returned nothing because the data had gone elsewhere.  I find myself always wanting that output.  That's what I meant by saying it was more string-like that stream-like.  But that behavior is really at the boundary between lldb & scripts, and can be controlled by the person providing the stream.  It is a little weird however that this behavior is non-explicit in SBStream.  W.R.T. below, I wonder if it wouldn't have been better to make the Stream output -> File behavior be something that has to be set at construction time and not modifiable.  It would be really surprising if I made an SBStream to gather a Description, and have somebody under the covers decide to drain the output off to a file.

> 
> 
>> In the case of HandleProcessEvent, SBDebugger uses it for HandleCommand, when it wants to dump all the stop event data to the Debugger's STDIO.  That would be less convenient with an SBStream, you'd have to make one, then Redirect it to the debugger's stdout & stderr.  That seems kind of awkward.
> 
> So, what would you say if we added a SBStream constructor taking an SBFile (which would do equivalent of `SBStream::RedirectToFile`). Then all HandleCommand would have to do is call `HandleProcessEvent(process, event, SBStream(GetOutputFile()), SBStream(GetErrorFile()))`. I think that would be fairly familiar to anyone who has worked with stream, as creating a stream which uses some other object (file, fd, string, ...) as a backing store is a pretty common pattern.

This would be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68546