[Lldb-commits] [PATCH] D82155: [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/include/lldb/Interpreter/CommandInterpreter.h:532
 
+  bool SaveTranscripts(std::string file_path);
+
----------------
 - Is there more than one transcript? 
 - Maybe make the string optional as you have logic to deal with that so you can call `SaveTranscripts()` instead of `SaveTranscripts("")` which looks a bit messy. 
 - Add a Doxygen comment documenting the empty-string behavior. 


================
Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:630
+
+  StreamString m_session_transcripts;
 };
----------------
The current name isn't very descriptive of what this object is exactly, especially since it's plural. How about `m_transcript_stream`? 


================
Comment at: lldb/source/Commands/CommandObjectSession.cpp:25
+    // argument entry.
+    arg1.push_back(channel_arg);
+
----------------
I changed the `CommandArgumentData` constructor so you can do 
```
arg1.emplace_back(eArgTypePath, eArgRepeatOptional)
```


================
Comment at: lldb/source/Commands/CommandObjectSession.h:16
+
+// CommandObjectSession
+
----------------
Redundant. 


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:2915
+             error_message, output_file, description);
+    StreamSP error_stream = GetDebugger().GetErrorStreamSP();
+    *error_stream << "Failed to save session's transcripts to " << output_file
----------------
Something that's called from a command should not write to the debugger's output/error stream directly. This should return an `Error` instead which then can be dealt with in the caller. If the caller is a CommandObject, it can write it to the return object. If the caller is something else it can still decide to write it to the debugger's error stream. 


================
Comment at: lldb/test/API/commands/session/save/TestSessionSave.py:42
+
+#        import pdb
+#        pdb.set_trace()
----------------
You probably didn't mean to leave this around. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82155