[Lldb-commits] [PATCH] D58564: [Reproducers] Add command provider

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

I have a couple of more comments, including some things I missed on the previous pass, but I don't want to hold this up any more. Feel free to commit after taking the last batch into consideration.



================
Comment at: lldb/include/lldb/Utility/Reproducer.h:116
+public:
+  DataRecorder(FileSpec filename, std::error_code ec)
+      : m_filename(std::move(filename)),
----------------
The error_code will need to be a reference argument for this to have any effect.


================
Comment at: lldb/source/API/SBDebugger.cpp:63
+public:
+  CommandLoader() {
+    repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
----------------
Use a factory function here too?


================
Comment at: lldb/source/API/SBDebugger.cpp:74
+    if (auto err = error_or_file.getError())
+      return;
+
----------------
It might be nice to log these errors even if we don't plan to do anything about them.


================
Comment at: lldb/source/Core/Debugger.cpp:887
+
 void Debugger::SetInputFileHandle(FILE *fh, bool tranfer_ownership) {
   if (m_input_file_sp)
----------------
Sticking with the "shadow" idea, I think it would be better if the input recorder is set as an extra argument to this function (instead of as a separate call), because they should always be changed together. Setting one without the other is almost certainly a mistake.


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

https://reviews.llvm.org/D58564