mib marked 10 inline comments as done.
mib 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
----------------
JDevlieghere wrote:
> I had a hard time understanding this sentence. Is the `crash information
> type` the key of the entry or the values in the array?
As I was trying to explain it on the diff summary, the "crash information type"
(i.e. `crash-info annotations) is the key of the dictionary entry and the value
is a dictionary array that contains all the entries for that specific crash
information type
================
Comment at: lldb/include/lldb/Target/Platform.h:881
const std::unique_ptr<ModuleCache> m_module_cache;
+ StructuredData::DictionarySP m_extended_crash_info;
----------------
JDevlieghere wrote:
> 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?
I did that so other platforms just need to implement
`FetchExtendedCrashInformation` and also to make sure the SBAPI method keeps
the same behaviour regardless of the platform.
================
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
----------------
JDevlieghere wrote:
> Assuming the header defines this struct, why not do
>
> ```
> #ifdef HAVE_CRASHREPORTERCLIENT_H
> #include <CrashReporterClient.h>
> #else
> struct CrashInfoAnnotations {
> ...
> }
> #endif
> ```
The struct have a different name in the header (which is not a big deal), but I
don't see the point of including the header if I give the structure definition
below it.
I put the header comment following @teemperor's feedback
(https://reviews.llvm.org/D74657#inline-679127) but maybe I could get rid of it
?
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
