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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
