[Lldb-commits] [PATCH] D73303: [lldb/Target] Add Abort StackFrame Recognizer

[Jonas Devlieghere via Phabricator via lldb-commits]

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