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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectSession.cpp:10
+public:
+  // Constructors and Destructors
+  CommandObjectSessionSave(CommandInterpreter &interpreter)
----------------
Remove


================
Comment at: lldb/source/Commands/CommandObjectSession.cpp:43
+  bool DoExecute(Args &args, CommandReturnObject &result) override {
+    llvm::StringRef file_path = "";
+
----------------
StringRef is empty by default, remove `= ""`


================
Comment at: lldb/source/Commands/CommandObjectSession.cpp:48
+
+    bool success = m_interpreter.SaveTranscripts(file_path);
+
----------------
Inline the variable below. 


================
Comment at: lldb/source/Commands/CommandObjectSession.cpp:64
+                 CommandObjectSP(new CommandObjectSessionSave(interpreter)));
+  //  TODO: Move 'history' subcommand from CommandObjectCommands
+}
----------------
Missing period.


================
Comment at: lldb/source/Commands/CommandObjectSession.h:20
+public:
+  // Constructors and Destructors
+  CommandObjectSession(CommandInterpreter &interpreter);
----------------
Remove


================
Comment at: lldb/source/Commands/CommandObjectSession.h:26
+private:
+  // For CommandObjectSession only
+  CommandObjectSession(const CommandObjectSession &) = delete;
----------------
Remove


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:124
+      m_command_source_depth(0), m_result(),
+      m_session_transcripts(new StreamString()) {
   SetEventName(eBroadcastBitThreadShouldExit, "thread-should-exit");
----------------
use `make_shared`


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2815
 
+  if (line.find("session save") == line.npos) {
+    *m_session_transcripts << io_handler.GetPrompt() << ' ' << line.c_str()
----------------
this won't work for things like unambiguous abbreviations like `sess save`. The command should do the saving.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2913
+bool CommandInterpreter::SaveTranscripts(llvm::StringRef file_path) {
+  std::string output_file = file_path.str();
+
----------------
If you're going to convert the StringRef to a std::string you might as well have that passed in by value so that the caller could move it when appropriate. 


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2929
+  Status error;
+  user_id_t fd_dst = FileCache::GetInstance().OpenFile(
+      FileSpec(output_file), flags, lldb::eFilePermissionsFileDefault, error);
----------------
Why are you going through the `FileCache`? 


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2941
+
+  if (fd_dst == UINT64_MAX || error.Fail())
+    return error_out("Unable to create file");
----------------
I think we have a constant LLDB_INVALID_FILE_DESCRIPTOR or something? If not you should use std::numeric_limits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82155