tammela added inline comments.
================
Comment at: lldb/bindings/lua/lua-wrapper.swig:21-22
+{
+ lldb::SBFrame sb_frame(stop_frame_sp);
+ lldb::SBBreakpointLocation sb_bp_loc(bp_loc_sp);
+
----------------
labath wrote:
> What's up with the copying? Is it to ensure the callback does not modify the
> caller's values? If so, you could just drop the `&` from the signature, and
> have the compiler copy these for you...
`SBFrame` and `SBBreakpointLocation` only provide copy constructors. I can't
see the difference between the two strategies, could you elaborate?
================
Comment at: lldb/bindings/lua/lua-wrapper.swig:25-26
+ // Get the Lua callback
+ lua_pushlightuserdata(L, baton);
+ lua_gettable(L, LUA_REGISTRYINDEX);
+
----------------
labath wrote:
> Is there a reason this function has to be in this file? It would be nice all
> the baton/registry handling was happening in the same place...
My reasoning was to have the `lua_pcall` on the same function that pushes the
Lua callback but I changed to your version as it's clearer who is handling the
baton (`Lua` class)
================
Comment at:
lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test:5
+b main
+breakpoint command add -s lua -o 'print(frame)'
+run
----------------
labath wrote:
> Could we also have a test where calling the callback raises an error? And for
> the different values of the "stop" argument that can be returned from the
> callback? And when compiling the callback expression results in an error?
Apparently when the user sets the breakpoint and it fails to compile the code
lldb will not report any errors and it will fail silently. I will address this
particular test case in another patch since it affects Python as well.
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
