[Lldb-commits] [PATCH] D82155: [WIP][lldb/interpreter] Add ability to save lldb session to a file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The code looks fine to me, though it sounds like there are still issues to be sorted out w.r.t commands run from inside data formatters, through sb apis, etc. And it needs tests, of course.



================
Comment at: lldb/source/Commands/CommandObjectSession.h:22-26
+  ~CommandObjectSession() override;
+
+private:
+  CommandObjectSession(const CommandObjectSession &) = delete;
+  const CommandObjectSession &operator=(const CommandObjectSession &) = delete;
----------------
You don't have to do this, but personally I'd delete all of this stuff -- the copy operations are implicitly deleted due to them being deleted in the base class, and the destructor will be implicitly overridden anyway.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2945
+  auto string_stream =
+      std::static_pointer_cast<StreamString>(m_session_transcripts);
+  size_t stream_size = string_stream->GetSize();
----------------
labath wrote:
> Why not make `m_session_transcripts` a `shared_ptr<StreamString>` (or even a plain `StreamString`) in the first place?
Much better, though a plain `StreamString` would be even nicer (and AFAICT there is nothing preventing that).


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2928
+  if (!opened_file)
+    return error_out("Unable to create file");
+
----------------
It would be nice if this error message actually contained the reason why the open operation failed (similar to what we did with the core file patch a while back).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82155