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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:317
+  else
+    stop_reason = stop_info_sp->GetStopReason();
+
----------------
jingham wrote:
> 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.
What about 
```
 StopReason stop_reason = eStopReasonNone;
  if (stop_info_sp)
    stop_reason = stop_info_sp->GetStopReason();
```


================
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);
----------------
jingham wrote:
> 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.
Can we make it at least `const` then? :-) 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73225/new/

https://reviews.llvm.org/D73225