JDevlieghere added inline comments.
================
Comment at: lldb/docs/use/formatting.rst:137
+---------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
-| ``thread.stop-reason`` | A textual reason each
thread stopped
|
+| ``thread.stop-reason`` | A textual reason why the
thread stopped. If the thread have a recognized frame, this displays its
recognized stop reason. Otherwise, gets the stop info description.
|
++---------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
----------------
This change seems somewhat orthogonal to this patch. Is the abort recognizer
the only one where these two will be different? If not I would suggest
splitting this off into a separate patch.
================
Comment at: lldb/include/lldb/Target/AbortRecognizer.h:33
+llvm::Optional<std::tuple<FileSpec, llvm::StringRef>>
+GetAbortLocation(Process *process_sp);
+
----------------
Why is this in the header? If this is only used by the implementation, you
should make these static function in the `cpp` file.
================
Comment at: lldb/include/lldb/Target/AbortRecognizer.h:48
+
+class AbortRecognizedStackFrame : public RecognizedStackFrame {
+public:
----------------
Doxygen comment?
================
Comment at: lldb/source/Core/FormatEntity.cpp:1279
Thread *thread = exe_ctx->GetThreadPtr();
if (thread) {
+ llvm::StringRef stop_description = thread->GetStopDescription();
----------------
I know you didn't touch this line but since you copied it below...
```
if(Thread *thread = exe_ctx->GetThreadPtr())
```
================
Comment at: lldb/source/Target/AbortRecognizer.cpp:42
+
+ /* We go a frame beyond the assert location because the most relevant
+ * frame for the user is the one in which the assert function was called.
----------------
(nit) We usually just use `//` for block comments.
================
Comment at: lldb/source/Target/AbortRecognizer.cpp:67
+ StringRef function_name;
+ std::tie(module_spec, function_name) = *assert_location;
+
----------------
I like `std::tie` but I wonder if a struct with named fields wouldn't make this
more readable with less code. Saves you six lines in this function and the next
one in exchange for a 4-line struct definition. Anyway I'll leave this up to
you to decide.
```
return lldb::RecognizedStackFrameSP(
new AbortRecognizedStackFrame(thread_sp, assert_location->module_spec,
assert_location->function_name));
```
================
Comment at: lldb/source/Target/AbortRecognizer.cpp:107
+
+ std::string module_name;
+ StringRef symbol_name;
----------------
Why is this a `std::string` and not a `StringRef`? I'm pretty sure `FileSpec`
has ctor that takes a StringRef.
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
