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

[Pavel Labath via Phabricator via lldb-commits]

labath 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);
+
----------------
tammela wrote:
> 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? 
Ok, let's try this.

My main point was that a function which receives an argument as a (non-const) reference, and then does not modify the referenced object is weird. One just doesn't do that. It should either take a const reference or a plain value. Using a plain value is simpler, particularly in cases where you need to locally modify the object you received, but you don't want the modifications to affect the callers value. That's because in this case the compiler will do the copying for you:
```
void foo(SBFrame foo, SBFrame &bar, const SBFrame &baz) {
  // I don't have to worry about modifying foo, because it's my local copy
  foo = ...;
  
  // Modifications to bar will be visible in the caller
  bar = ...;

  // I can't modify baz. If I really need to do that, I need to make a local copy
  SBFrame baz_copy = baz;
  baz_copy = ...;
}
```


================
Comment at: lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test:10
+run
+# CHECK: frame #0
+# CHECK: Process {{[0-9]+}} exited with status = 0
----------------
"frame #0" will also get printed if the process stops for any reason. I'd suggest printing a completely random string instead (`print("bacon")`, for example)


================
Comment at: lldb/test/Shell/ScriptInterpreter/Lua/breakpoint_oneline_callback.test:5
+b main
+breakpoint command add -s lua -o 'print(frame)'
+run
----------------
tammela wrote:
> 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.
Sounds good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91508/new/

https://reviews.llvm.org/D91508