friss added inline comments.
================
Comment at: lldb/source/Target/AbortRecognizer.cpp:126-127
+ // Fetch abort location
+ auto abort_location =
+ AbortRecognizerHandler::GetAbortLocation(process_sp.get());
+
----------------
Why do you even check the abort location? The recognizer has been registered
for this specific module and function, so you're never going to get here with a
different top frame.
================
Comment at: lldb/source/Target/Process.cpp:543
+
+ RegisterAbortRecognizer(this);
}
----------------
We need to find a better place to register this.
================
Comment at: lldb/source/Target/Process.cpp:945
+
+ uint32_t mrf_idx = 0;
+
----------------
This variable name just looks weird. I know what it is in the context of this
patch, but I would never get it re-reading this code a year from now. Just
hoist the `start_frame` variable from bellow and use it.
================
Comment at: lldb/source/Target/Process.cpp:947
+
+ auto frame_sp = cur_thread->GetStackFrameAtIndex(mrf_idx);
+
----------------
No point in using the variable here. You want the top frame, let the code
reflect this and just use 0.
================
Comment at: lldb/source/Target/Process.cpp:956
const bool only_threads_with_stop_reason = true;
- const uint32_t start_frame = 0;
+ const uint32_t start_frame = mrf_idx;
const uint32_t num_frames = 1;
----------------
Huh, reading further down the patch, it feels pretty wrong that you have to
compute the "most relevant frame" here and in `WillStop()`. Should this just be
something like `start_frame = cur_thread->GetSelectedFrame()->GetIndex()`?
================
Comment at: lldb/source/Target/Thread.cpp:576
+void Thread::ApplyMostRelevantFrames() {
+ if (!m_curr_frames_sp)
----------------
Why does this function name use a plural form? Wouldn't
`SelectMostRelevantFrame` be a more explicit name?
================
Comment at: lldb/source/Target/Thread.cpp:581
+ auto cur_frame = m_curr_frames_sp->GetFrameAtIndex(0);
+ SymbolContext sym_ctx =
cur_frame->GetSymbolContext(eSymbolContextEverything);
+
----------------
Unused variable?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73303/new/
https://reviews.llvm.org/D73303
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
