Greg should probably take a look as well, but this seems fine to me. There are
a couple of typos in the comments, e.g.
// check if the error was something other then an unsupported
breakpoint type
the "then" should be "than".
// we will reach here when the stub gives an unsported response to a
hardware breakpoint
That should be "unsupported".
And in general we try to write comments as whole sentences with initial
capitals and punctuation, but on the other hand you've done a great job
explaining the method so I can't get too sticky about that.
The "unavailable" in this error:
error.SetErrorString("failed to set hardware breakpoint (hardware
breakpoint resources might be exhausted or unavailable)");
is a little confusing to me. You already know the target says it supports
hardware breakpoints so I'm not sure what unavailable means.
BTW, I think the SupportsGDBStoppointPacket interface would be easier to
understand if it was returning LazyBool's rather than returning a bool where
"true" might mean "dunno" or it might mean "true". This code was probably
written before we added the LazyBool's so it goes this route instead. I don't
think it is necessary to rewrite your patch to use them, it's not hard to
follow and you've explained it well. But for future reference, that's a better
way to handle this tri-state of "yes", "no" and "dunno".
Thanks for working on this.
Jim
On Feb 21, 2014, at 5:26 AM, Deepak Panickal <[email protected]> wrote:
> This addresses the concerns raised by Greg and Jim. Fixed the patch so that
> if software breakpoints are supported, we do not fall through to other
> breakpoint types. Only if it's not supported, we try the next option.
>
> http://llvm-reviews.chandlerc.com/D2826
>
> CHANGE SINCE LAST DIFF
> http://llvm-reviews.chandlerc.com/D2826?vs=7207&id=7278#toc
>
> Files:
> source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
> source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
> <D2826.2.patch>_______________________________________________
> 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
