JDevlieghere added inline comments.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:786
+
+ m_opt_dispatch_map.insert(std::pair<lldb::addr_t, int>(sym_addr, i));
+ }
----------------
You could simplify this by using `.emplace(sym_addr, i)`.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:889
+ pos = m_msgSend_map.find(addr);
+ if (pos != m_msgSend_map.end()) {
+ return_ptr = &g_dispatch_functions[(*pos).second];
----------------
If you rewrite this you can get rid of the return_ptr temporary.
```
if (pos != m_msgSend_map.end()) {
return &g_dispatch_functions[(*pos).second];
}
return nullptr;
```
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:918
- MsgsendMap::iterator pos;
- pos = m_msgSend_map.find(curr_pc);
- if (pos != m_msgSend_map.end()) {
- this_dispatch = g_dispatch_functions[(*pos).second];
- found_it = true;
- }
-
+ this_dispatch = FindDispatchFunction(curr_pc);
+
----------------
Can you initialize this on line 910?
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:1200
+ if (!this_dispatch && !ret_plan_sp) {
+ const char *opt_name = nullptr;
+ MsgsendMap::iterator pos;
----------------
Same here, why not do the assignment when you declare the variables?
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:202
+
+// objc uses optimized dispatch functions for some common and seldom overridden
+// methods. For instance
----------------
`/// Objective-C ...`
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:317
+ else
+ stop_reason = stop_info_sp->GetStopReason();
+
----------------
```
StopReason stop_reason = stop_info_sp ? stop_info_sp->GetStopReason() :
eStopReasonNone;
```
================
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();
----------------
I'd invert the condition and make this an early return with the comment below.
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:358
+bool AppleThreadPlanStepThroughDirectDispatch::ShouldStop(Event *event_ptr) {
+
+ // If step out plan finished, that means we didn't find our way into a
method
----------------
Spurious empty line
================
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);
----------------
Looks like `step_out_should_stop ` isn't used anywhere else?
```if(ThreadPlanStepOut::ShouldStop(event_ptr)) {```
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:67
lldb::addr_t m_sel_addr;
lldb::ThreadPlanSP m_func_sp; // This is the function call plan. We fill it
+ // at start, then set it to NULL when this plan
----------------
These should be Doxygen comments before the variable if you're already touching
them...
```
/// This is the function call plan. We fill it at the start, then ...
lldb::ThreadPlanSP m_func_sp;
```
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:77
+class AppleThreadPlanStepThroughDirectDispatch
+ : public ThreadPlanStepOut {
----------------
Can you please clang-format the patch, this should fit on one line.
================
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.
----------------
`///`
================
Comment at:
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:110
+ std::vector<lldb::BreakpointSP> m_msgSend_bkpts;
+ bool m_at_msg_send = false;
+ bool m_stop_others;
----------------
Let's initialize this in the ctor as well, the inconsistency with the next line
leaves me wondering if this is a bug.
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
