labath added inline comments.
================
Comment at: lldb/bindings/lua/lua-wrapper.swig:26-27
+
+ SBTypeToSWIGWrapper(L, &sb_frame);
+ SBTypeToSWIGWrapper(L, &sb_bp_loc);
+
----------------
This name made sense for python, as the functions actually returned the
wrappers. But here, the name makes it pretty unobvious that these functions
actually push the object on the lua stack. It'd be better if this was called
something like `PushSBType` or something
================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:60-78
+static int runCallback(lua_State *L) {
+ LuaCallback *callback = static_cast<LuaCallback *>(lua_touserdata(L, -1));
+ return (*callback)(L);
+}
+
+llvm::Error Lua::Run(LuaCallback &callback) {
+ lua_pushcfunction(m_lua_state, runCallback);
----------------
tammela wrote:
> labath wrote:
> > I'm confused. Why use lua to call a c callback, when you could just do
> > `calllback()`?
> Some Lua API functions throw errors, if there's any it will `abort()` the
> program if no panic handler or protected call is provided.
> To guarantee that the callback always runs in a protected environment and it
> has error handling, we do the above.
> Delegating this to the callback itself makes it cumbersome to write.
Aha, I see.
So, if I remember my lua correctly (I wrote a c++ lua wrapper once, but that
was years ago..), whenever there's a lua exception inside this (c++) callback,
lua will longjmp(3) back to the lua_pcall call on line 68, skipping any
destructors that should normally be invoked. Is that so?
If that's the case, then I think this is a dangerous api, that should at the
very least get a big fat warning, but that ideally shouldn't exist at all.
What's the part that makes delegating this to the callback "cumersome to
write"? And why can't that be overcome by a suitable utility function which
wraps `lua_pcall` or whatever else is needed?
The approach that we've chosen in python is to have very little code
interacting with the python C API directly. Instead, code generally works with
our C++ wrappers defined in `PythonDataObject.h`. These functions try to hide
the python exception magic as much as possible, and present a c++-y version of
the interface.
Now, since lua is stack-based, something like LuaDataObjects.h would probably
not work. However, that doesn't meant we should give up on the c++ wrapper
idea altogether. During the intitial reviews, my intention was for the `Lua`
class to serve this purpose. I still think this can be achieved if we make the
callback functions take `Lua&` instead of `lua_State*` as an argument, and then
ensure the class contains whatever is needed to make the callbacks not
cumerbsome to write.
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:200
+ Debugger &debugger = target->GetDebugger();
+ ScriptInterpreterLua *lua_interpreter =
+ static_cast<ScriptInterpreterLua *>(debugger.GetScriptInterpreter());
----------------
tammela wrote:
> labath wrote:
> > How is `lua_interpreter` different from `this`?
> Are you asking why I didn't go for `m_script_interpreter`?
>
> This function is static (on the class declaration), perhaps it's clearer a
> `static` keyword here as well?
I just missed the fact that this is a static function (I hate these static
baton functions in lldb). Putting `static` on an out-of-line member function
definition is not valid c++.
However, this does raise a different issue. Debugger::GetScriptInterpreter will
return the default script interpreter, and that may not be the lua one. I think
you should pass eScriptLanguageLua here explicitly.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91508/new/
https://reviews.llvm.org/D91508
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
