jingham added a comment.
In D68671#1704803 <https://reviews.llvm.org/D68671#1704803>, @clayborg wrote:
> 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?
Since we still support the form that doesn't take extra_args, they do feel
"extra" to me. I also like the "extra" because it helps make it clear these
are just free bits of data that the callback system just passes along. After
all, there are some fixed arguments to the call back that do have specific
meanings.
Note, I called the equivalent feature "extra_args" for the scripted breakpoint
resolvers (added last year) and scripted thread plans, so if we change it here,
we need to change it in those places as well. If we're going to do that, I'll
do it as a follow on patch because I don't want to mix those changes in with
this patch. But as I said, I like the "extra".
> - 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.
The only place where I treat the extra_args as a Dictionary is when I use as a
way to tell whether the extra_args is empty, which I need to write the right
wrapper function in GenerateBreakpointCommandCallbackData. That function is
little annoying because it doesn't know whether it is calling a function or
some text gotten from the I/O handler, so I can't count the function arguments
there to decide which signature to use.
Let me see if I can make this cleaner so I don't have to rely on that. I
really don't intend to limit fancier uses of the extra args.
Note, the command-line doesn't allow you to make anything but a dictionary
(through the -k -v option pairs). I'm actually happy with that, it enforces
some kind of structure on the breakpoint command callback that you write.
You'll thank me when you want to add a second or third parameter...
Plus once you write something you are going to use frequently, you can do:
(lldb) command alias caller_break breakpoint command add -F
MyCommands.break_when_parent -k func_name -v %1
(lldb) caller_break Caller -n Callee
That's how you would really want to use it.
> - 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?
There's no way to do this. "breakpoint command add" is a bit poorly named,
since it really overwrites the current command rather than adding to it. But
that's the way it has always worked. So only giving you the option to provide
a new pair {python function, extra_args} is in keeping with its behavior.
People have asked for a "breakpoint command modify" that could chain python
callbacks, or incrementally add command-line commands. That would also
naturally allow you to change/add to the extra args in place. But that's a
whole other piece of work that I don't want to take on here.
> - 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?
I thought about adding --json when making the OptionGroupPythonClassWithDict.
The completionist side of me was sorely tempted, especially since it would be
quite easy to do. But the options for "break set" are already several pages
long, so I don't want to add more options to it just "because I can".
For all the use cases I can think of, a simple one level dictionary will
suffice. If you want to do something fancier, you can make an arbitrarily
complicated SBStructuredData in Python, and then use
SBBreakpoint.SetScriptCallbackFunction to pass it in. And if you want to do
that something fancier a lot, you can make this into a python command.
One useful enhancement that wouldn't overburden "break set" would be to call
ParseJSON on the -v option value, and if that succeeded add the resultant
StructuredData::ObjectSP as the value. That would allow you to pass lists in
for the value, which does seem useful. You can of course do this yourself by
getting the string value out in your callback and passing it to
SBStructuredData.ParseFromJSON(). So I don't think it's important to add this
now. I'd also need to assure myself that this wouldn't surprise people by
munging up legitimate string values.
But anyway, I'd prefer to do that as a follow on, since I'm running out of time
to play with this this time round the wheel...
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68671/new/
https://reviews.llvm.org/D68671
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
