JDevlieghere added inline comments.
================
Comment at: lldb/include/lldb/Target/Platform.h:838
+ /// \return
+ /// A Structured Data Dictionnary containing at each entry an array for
+ /// each different crash information type. Or a nullptr if the process
has
----------------
I had a hard time understanding this sentence. Is the `crash information type`
the key of the entry or the values in the array?
================
Comment at: lldb/include/lldb/Target/Platform.h:881
const std::unique_ptr<ModuleCache> m_module_cache;
+ StructuredData::DictionarySP m_extended_crash_info;
----------------
I find it suspicious that this member is stored here but not used. Does it need
to be stored in the first place? Given the FetchExtendedCrashInformation I'd
expect these dictionaries to be different per target?
================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1520
+// CommandObjectPlatformProcessCrashInfo
+#pragma mark CommandObjectPlatformProcessCrashInfo
----------------
This comment is useless.
================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1521
+// CommandObjectPlatformProcessCrashInfo
+#pragma mark CommandObjectPlatformProcessCrashInfo
+
----------------
As I've said in other reviews I really dislike the `#pragma mark`. It's fine if
the file already uses them but I really don't want to encourage new ones being
added.
================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1541
+ if (!platform_sp) {
+ result.AppendError("Couldn'retrieve the selected platform");
+ return result.Succeeded();
----------------
Shouldn't you update the return status? Same below.
================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:1514
+ LLDB_LOG(log, "Couldn't extract crash information annotations");
+ }
+
----------------
The braces are inconsistent with the single-line ifs below. Why not wrap this
log statement in the if check below?
```
if (!annotations)
LLDB_LOG(log, "Couldn't extract crash information annotations")
else
m_extended_crash_info->AddItem("crash-info annotations", annotations);
```
If you want the log statement to stay close to the
`ExtractCrashInfoAnnotations` call (which I think you should), you could move
the `!m_extended_crash_info` check up.
================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:1596
+
+ if (!annotations.message2) {
+ LLDB_LOG(log, "No message2 available for module {0}.", module_name);
----------------
braces
================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h:93
+ // This struct should only be available in macOS but could be used on other
+ // platforms. #ifdef HAVE_CRASHREPORTERCLIENT_H #include
+ // <CrashReporterClient.h> #endif
----------------
Assuming the header defines this struct, why not do
```
#ifdef HAVE_CRASHREPORTERCLIENT_H
#include <CrashReporterClient.h>
#else
struct CrashInfoAnnotations {
...
}
#endif
```
================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h:124
+ ExtractCrashInfoAnnotations(lldb_private::Target &target,
+ lldb_private::Log *log);
+
----------------
The log is a singleton, why pass it around?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74657/new/
https://reviews.llvm.org/D74657
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
