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

[Jim Ingham via Phabricator via lldb-commits]

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