[Lldb-commits] [PATCH] D12360: RenderScript pending kernel breakpoints.

[Jim Ingham via lldb-commits]

jingham added a comment.

That looks great.  Here are some comments that are not requirements for getting this in but just for you to consider:

If you think you are ever likely to add different ways to specify the name (e.g. if passing a regex might be useful) it might be good to pass the name as an option (-n FOO) rather than a bare argument.  Once you've made it an argument it is hard to add orthogonal options in a coherent way.  OTOH, in the case of regex, you could for instance do:

(lldb) renderscript kernel breakpoint -r <NAME>

and actually we do have some commands that do it that way ("image lookup" being one) but since this is kind of a breakpoint command it would be better to make it look like the "breakpoint set" command.  But that only works because a regex is just a fancy name somehow.  Anyway, there are also other people around who wish I'd lay off the option thing a bit, so take this suggestion as you will...

I don't know if it is useful to you, but all the "by name" breakpoint commands also take a list of names, so you can do:

(lldb) break set -n Foo -n Bar -n Baz

That's useful if you want to have some devious command on all three of those breakpoints, or want to enable & disable them at a blow, or if you are interested in the aggregated hit count for the three or whatever.  Be pretty easy to do that the way you've got it set up now.

Also, if you get some extra time, it doesn't look like it would be too hard to add a command completer for this name argument.  It looks like you would just run over the modules looking for kernels, then match the input against those names.  That would get tab completion on the name working automatically.  This is extra credit, however.


Repository:
  rL LLVM

http://reviews.llvm.org/D12360