JDevlieghere added inline comments.
================
Comment at: lldb/include/lldb/Target/AbortRecognizer.h:27
+
+ static llvm::Optional<std::tuple<FileSpec, ConstString>>
+ GetAbortLocation(Process *process_sp);
----------------
This doesn't really look much like a class with just two static member
functions. Assuming that the `Process` is going to be the same for both, maybe
store that as a member? Otherwise you might be better off having them as static
functions in the implementation.
================
Comment at: lldb/include/lldb/Target/AbortRecognizer.h:33
+
+#pragma mark Frame recognizers
+
----------------
Although LLDB already has a lot of these markers, I don't think we want to add
more of them. They're very specific to a single IDE available on a single
platform :-)
================
Comment at: lldb/include/lldb/Target/Thread.h:219
+ void ApplyMostRelevantFrames();
+
----------------
What does it mean to "apply" a frame? I think this needs a comment with some
explanation or a better name.
================
Comment at: lldb/source/Target/AbortRecognizer.cpp:25
+
+llvm::Optional<std::tuple<FileSpec, ConstString>>
+AbortRecognizerHandler::GetAbortLocation(Process *process) {
----------------
Why does this need to return a `ConstString`?
================
Comment at: lldb/source/Target/AbortRecognizer.cpp:35
+ case llvm::Triple::MacOSX:
+ module_name = "libsystem_kernel.dylib";
+ symbol_name = "__pthread_kill";
----------------
Personally I'd return here:
```
return std::make_tuple(FileSpec(module_name), symbol_name);
```
which would make it possible to...
- get rid of the temporary `std::string`s,
- make the second element of the tuple just a StringRef.
================
Comment at: lldb/source/Target/AbortRecognizer.cpp:51
+
+llvm::Optional<std::tuple<FileSpec, ConstString>>
+AbortRecognizerHandler::GetAssertLocation(Process *process) {
----------------
Same comments as the previous method.
================
Comment at: lldb/source/Target/AbortRecognizer.cpp:81
+ ThreadSP thread_sp, FileSpec module_spec, ConstString function_name) {
+ const uint32_t frames_to_fetch = 10;
+ StackFrameSP prev_frame_sp = nullptr;
----------------
Magic value? Why 10?
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
