Hi Jim,
Thanks for looking through.
I've fixed the typos and uploaded a new patch.
Will commit once Greg takes a look.
Thanks,
Deepak
On 21/02/2014 17:22, [email protected] wrote:
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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
