[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Looks good overall. A few questions:

- should we do a global "s/extra_args/args/" edit in this patch? Not sure what "extra_" is adding? Can we remove?
- Can the structured data not be a dictionary? Valid StructuredData could be "malloc" or "[0x1000, 0x2000]" or "true", etc. If the data is required to be a dictionary we should provide lldb::SBError support for the new API calls to ensure we can let the user know what they did wrong. Be good to test this as well.
- is it possible to update the args for a callback function? That would seem like a good thing you might want to do? And if so, to test?
  - If it is possible to update the structured data, and if the structured data can be anything (array, string, boolean, dictionary), we might want an option like: --json "..." to be able to set or update from the command line? The current -k -v options seem to imply it must be a dict and only one level deep?



================
Comment at: lldb/include/lldb/API/SBBreakpointLocation.h:59
+  void SetScriptCallbackFunction(const char *callback_function_name,
+                                 SBStructuredData &extra_args);
+
----------------
shafik wrote:
> Could this be a `const &`
const doesn't mean anything for lldb::SB arguments. Since they contain shared pointers or unique pointers, so just leave as is.


================
Comment at: lldb/include/lldb/API/SBBreakpointName.h:89
+  void SetScriptCallbackFunction(const char *callback_function_name,
+                                 SBStructuredData &extra_args);
+
----------------
shafik wrote:
> Could this be a `const &`
const doesn't mean anything for lldb::SB arguments. Since they contain shared pointers or unique pointers, so just leave as is.


================
Comment at: lldb/include/lldb/Interpreter/OptionGroupPythonClassWithDict.h:26-30
   OptionGroupPythonClassWithDict(const char *class_use,
+                      bool is_class = true,
                       int class_option = 'C',
                       int key_option = 'k', 
+                      int value_option = 'v');
----------------
clang-format? Indent seems off (it was before too I see).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671