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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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 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.



================
Comment at: lldb/source/API/SBProcess.cpp:359-364
     char message[1024];
-    int message_len = ::snprintf(
+    size_t message_len = ::snprintf(
         message, sizeof(message), "Process %" PRIu64 " %s\n",
         process_sp->GetID(), SBDebugger::StateAsCString(event_state));
-
     if (message_len > 0)
+      out->Write((void *)message, message_len);
----------------
For instance, this code already kind of mocks a "stream wrapping a file object" by using snprintf. If we had a real stream object around (which properly supported formatted IO) then this could be simplified to `stream->Printf(...)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68546