Richard,
Thanks for the patch. I have a few comments.
1) Why do you allow multiple files to be specified on input? You don't seem to
do anything with anything but the first file, so this just adds complexity to
the code, and makes the workings of the command less clear. The error will
also be weird if you supplied two files, but
Note, one of the things that is currently missing from the Command Interpreter
is a way to specify whether multiple entries of some option are allowed or not.
So for now you have to handle this by hand, by checking whether your m_file
ivar is empty on assign and erroring out of the parsing if not.
2) I try not to write commands that don't report parse errors in input
immediately. So for instance, I would do:
case 'l':
bool success;
size_t tmp_line_num = Args::StringToUInt32( option_arg, 0,
0, &success);
if (!success)
error.SetErrorStringWithFormat("invalid line number:
\"%s\"", option_arg);
else
m_line_num = tmp_line_num;
break;
that way you won't end up getting into funny situations when some option
defaults back to 0 because the input was wrong.
3) I'm not sure that doing a generic file & line number search using the
AddressResolverFileLine is the most efficient way to do what you want. That is
going to search all the line tables in all the modules loaded in the program,
which could be quite a large amount of data. Plus, you know in advance that
you are only interested in the specified file & line in the current frame's
function. It might be much easier to run through the line table for the
current function for a file & line match.
There isn't a pre-packaged way to get the beginning and ending line table
entries for a given function, but it is pretty easy to do. You have the
current function already, so get the CompileUnit from that, get its line table,
and use LineTable::FindLineEntryByAddress - passing in the base address of the
function's AddressRange - to get the entry & index for the start line table
entry. Then iterate through the line table using the index till the address is
past the end address of the function.
Anyway, if you want to keep with your current scheme, you can certainly make a
SearchFilter for the module containing the current frame's function, that will
narrow down the search considerably.
4) This is minor, but the way you have written it, if there are multiple
instances of the file/line in the current function, you go to the first one
without any warning. I don't know if there's any better choice for where to
go, but maybe you want to at least warn in that case.
5) This is extra credit, but we try not to have useful operations that can be
done with the command line but not with the SB API's or vice versa. Seems like
it would be pretty natural to add an SBFrame::JumpPC alongside SBFrame::SetPC.
Again, thanks for working on this.
Jim
On Aug 23, 2013, at 12:34 PM, Richard Mitton <[email protected]> wrote:
>
> <ping/>
>
> So, I just wanted to check here; the LLVM developer policy says I'm
> expected to get every patch reviewed before submission. In general, for
> lack of any feedback, is it OK to just submit stuff like this without
> approval?
>
> Richard Mitton
> [email protected]
>
> http://llvm-reviews.chandlerc.com/D1452
> _______________________________________________
> lldb-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
