[Lldb-commits] [PATCH] D74657: [lldb/Plugins] Add `platform process crash-info` command

[Med Ismail Bennani via Phabricator via lldb-commits]

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