labath added a comment.
This is starting to look pretty good. I think that pretty soon you'll have to
come up with some way of having a more persistent lua context (instead of
creating a fresh one for each command), but that doesn't have to happen now.
Does the "multiline" script command do anything already? Is there anything to
test with respect to that.
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:19-21
+#ifndef LLDB_DISABLE_LIBEDIT
+#include "lldb/Host/Editline.h"
+#endif
----------------
No longer needed?
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:25
+
+#define LUA_PROMPT ">>> "
+
----------------
it doesn't look like this is used (but if it is, it would be better off as a
regular variable)
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:59
+private:
+ lua_State *m_lua_state = nullptr;
+};
----------------
The constructor initialized this member, and the desctructor asserts it to be
not null. It doesn't look like this assignment here is actually helping
anything..
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:74
+ if (llvm::Error error = m_lua.Run(data)) {
+ fprintf(GetOutputFILE(), "%s", llvm::toString(std::move(error)).c_str());
+ }
----------------
*GetOutputStreamFileSP() << llvm::toString(std::move(error))
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:78
+
+ ~IOHandlerLuaInterpreter() override {}
+
----------------
delete
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:118
+ IOHandlerSP io_handler_sp(new IOHandlerLuaInterpreter(debugger));
+ if (io_handler_sp) {
+ debugger.PushIOHandler(io_handler_sp);
----------------
This if is not needed (new always succeeds)
================
Comment at: lldb/unittests/ScriptInterpreter/Lua/LuaTests.cpp:53
+TEST_F(ScriptInterpreterTest, ExecuteOneLine) {
+ DebuggerSP debugger_sp = Debugger::CreateInstance();
+ ASSERT_TRUE(debugger_sp);
----------------
I'm not actually opposed to this (and it's nice to know that this could work if
we need it), but why do this as a unit test? It seems like this could be tested
equally well with a `script` command through the lldb driver. I was imagining
the unit tests would be most useful for the lower level functionality --
roughly corresponding to the `Lua` class. Right now that class is pretty simple
and doesn't really need a unit test, but I expect it will become more complex
over time...
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71234/new/
https://reviews.llvm.org/D71234
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
