jingham marked 13 inline comments as done.
jingham added a comment.
Thanks for the suggestions! A couple were not to my taste, but I fixed all the
others.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:317
+ else
+ stop_reason = stop_info_sp->GetStopReason();
+
----------------
JDevlieghere wrote:
> ```
> StopReason stop_reason = stop_info_sp ? stop_info_sp->GetStopReason() :
> eStopReasonNone;
> ```
I don't like trigraphs like this. The don't make things clearer to my eye and
if you ever wanted to stop on one or the other branches, you are just sad.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:320
+ // See if this is one of our msgSend breakpoints:
+ if (stop_reason == eStopReasonBreakpoint) {
+ ProcessSP process_sp = GetThread().GetProcess();
----------------
JDevlieghere wrote:
> I'd invert the condition and make this an early return with the comment
> below.
The logic in this computation is generally goes "if A do X, else if B do Y"...
So I don't like to write these:
if (!The One Case I've Had To Handle So Far)
return;
DoStuff();
It isn't really that much clearer, and it means you have to redo it to handle
another case.
So I'm happy with it the way it is.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:364
+ bool step_out_should_stop = ThreadPlanStepOut::ShouldStop(event_ptr);
+ if (step_out_should_stop) {
+ SetPlanComplete(true);
----------------
JDevlieghere wrote:
> Looks like `step_out_should_stop ` isn't used anywhere else?
>
> ```if(ThreadPlanStepOut::ShouldStop(event_ptr)) {```
I don't see the benefit. What's there is easy to read, and more
self-documenting. Having important bits of code in an if statement always
makes them harder to read for me. I'm happy the way this is.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:93
+
+ // The base class MischiefManaged does some cleanup - so you have to call it
+ // in your MischiefManaged derived class.
----------------
JDevlieghere wrote:
> `///`
This wasn't doc, it was a reminder to me, but it wasn't necessary so I removed
it...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73225/new/
https://reviews.llvm.org/D73225
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
