[Lldb-commits] [PATCH] D91508: [LLDB/Lua] add support for one-liner breakpoint callback

[Pedro Tammela via Phabricator via lldb-commits]

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