[Lldb-commits] [PATCH] D82396: [lldb/ScriptInterpreter] Extract IO redirection logic and move it out of ScriptInterpreterPython (NFC)

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

I think this looks good, though it looks like you have uploaded an partial patch...



================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:57-58
 private:
+  friend std::unique_ptr<ScriptInterpreterIORedirect>
+  std::make_unique<ScriptInterpreterIORedirect>();
+
----------------
If that works, I suppose it's fine. But I wouldn't be surprised if this trick backfires on some STL implementations.

I think that making an exception for make_unique on classes with private constructors is fine (we already have a bunch of classes like that)...


================
Comment at: lldb/source/Interpreter/ScriptInterpreter.cpp:128-168
   if (result) {
-    m_input_file_sp = debugger.GetInputFileSP();
+    redirect->m_input_file_sp = debugger.GetInputFileSP();
 
     Pipe pipe;
     Status pipe_result = pipe.CreateNew(false);
 #if defined(_WIN32)
     lldb::file_t read_file = pipe.GetReadNativeHandle();
----------------
Given that none of this fails (though maybe it should), I think it would be cleaner to keep it in the constructor. You can still keep the static factory thing as a wrapper if you want symmetry.


================
Comment at: lldb/source/Interpreter/ScriptInterpreter.cpp:184-190
+  std::unique_ptr<ScriptInterpreterIORedirect> redirect =
+      std::make_unique<ScriptInterpreterIORedirect>();
+  redirect->m_input_file_sp = std::move(nullin.get());
+  redirect->m_error_file_sp = redirect->m_output_file_sp =
       std::make_shared<StreamFile>(std::move(nullout.get()));
+
+  return redirect;
----------------
Similarly, I would find this better as `return std::make_unique<ScriptInterpreterIORedirect>(std::move(nullin.get()), std::move(nullout.get()))` (with an appropriate constructor. Mainly because then I don't need to worry about what will the destructor of ScriptInterpreterIORedirect do if we return with the object in an partially initialized state.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D82396