[Lldb-commits] [PATCH] D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods

Jim Ingham via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Jan 22 15:43:36 PST 2020

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)


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...

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list