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