[Lldb-commits] [PATCH] D71234: [lldb/Lua] Implement a Simple Lua Script Interpreter Prototype

[Pavel Labath via Phabricator via lldb-commits]

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