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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
