teemperor added inline comments.
================ Comment at: lldb/include/lldb/Target/AbortRecognizer.h:29 + GetAbortLocation(Process *process_sp); + static llvm::Optional<std::tuple<FileSpec, ConstString>> + GetAssertLocation(Process *process_sp); ---------------- This function (especially the return types) deserve some documentation. ================ Comment at: lldb/source/Target/AbortRecognizer.cpp:1 +//===-- AbortRecognizer.cpp -------------------------------------*- C++ -*-===// +// ---------------- Please remove the `-*- C++ -*-` as that's only for header files. ================ Comment at: lldb/source/Target/AbortRecognizer.cpp:85 + // Fetch most relevant frame + for (uint32_t i = 0; i < frames_to_fetch; i++) { + prev_frame_sp = thread_sp->GetStackFrameAtIndex(i); ---------------- Could we make this `frame_index` instead of just `I`. I was confused that `I` isn't just an index into a vector when you later interpret it as a stack frame index. ================ Comment at: lldb/source/Target/AbortRecognizer.cpp:99 + if (sym_ctx.module_sp->GetFileSpec().GetFilename() == + module_spec.GetFilename() && + sym_ctx.GetFunctionName() == function_name) { ---------------- You can just do `sym_ctx.module_sp->GetFileSpec().FileEquals(module_spec)` which also handles case insensitive file systems IIRC. ================ Comment at: lldb/source/Target/AbortRecognizer.cpp:101 + sym_ctx.GetFunctionName() == function_name) { + if (i < frames_to_fetch - 1) + m_most_relevant_frame = thread_sp->GetStackFrameAtIndex(i + 1); ---------------- Maybe add a comment why you need to skip one frame. ================ Comment at: lldb/source/Target/AbortRecognizer.cpp:104 + else + m_most_relevant_frame = thread_sp->GetStackFrameAtIndex(i); + break; ---------------- Maybe something like this? Just a suggestion though. ``` lang=c++ uint32_t last_frame_index = frames_to_fetch - 1; m_most_relevant_frame = thread_sp->GetStackFrameAtIndex(std::min(i + 1, last_frame_index)); ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73303/new/ https://reviews.llvm.org/D73303 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits