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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 9 inline comments as done.
mib 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.                                                                                                                        |
++---------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
----------------
JDevlieghere wrote:
> 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.  
It wouldn't make sense to split this into different patches since I'm only changing the current `thread.stop-reason` in this one. And for users who still want the original behaviour, I added `thread.stop-reason-raw`.


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:67
+  StringRef function_name;
+  std::tie(module_spec, function_name) = *assert_location;
+
----------------
JDevlieghere wrote:
> 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));
> ```
> 
I changed the implementation so I'm only passing 1 parameter (the most relevant frame) to  the constructor.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73303/new/

https://reviews.llvm.org/D73303