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

[Jonas Devlieghere via Phabricator via lldb-commits]

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
----------------
mib wrote:
> 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
Would you mind updating the comment to explain it that way? 


================
Comment at: lldb/include/lldb/Target/Platform.h:881
   const std::unique_ptr<ModuleCache> m_module_cache;
+  StructuredData::DictionarySP m_extended_crash_info;
 
----------------
mib wrote:
> 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.
I'm not sure I understand how that answers my questions. The SB API calls FetchExtendedCrashInformation, what does it care about the member? Why not return a new DictionarySP every time?


================
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
----------------
mib wrote:
> 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 ?
> 
> 
I'll leave it up to your discretion, but I'd either specify the struct myself and say it's defined in `CrashReporterClient.h`, or alternatively put the ifdefs in the code and include the header and typedef it if the header is available, and use the custom struct otherwise, which means we still have the layout and type info Raphael asked about. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74657