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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to