labath added a comment.
This looks a bit better, but my the purpose of my previous command was not to
move the location of the cli command itself -- I think it was ok just where it
was. The fact that the part of the functionality is implemented in the Platform
class does not mean the cli command has to be there.
================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1574-1577
+ interpreter, "platform process",
+ "Commands to query, launch and attach to "
+ "processes on the current platform.",
+ "platform process [attach|launch|list|crash-info] ...") {
----------------
I am not sure if this command really belongs here. If definitely doesn't fit
the description "Commands to query, launch and attach to processes on the
current platform.", and if you look at the subcommands, you see that these are
all things that you do *before* you get an actual running process. "crash-info"
is the exact opposite -- it only makes sense *after* you have a process.
The "process" (where you had it originally) command tree contains commands for
interaction with a running process (although it also overlaps these commands
somewhat :/). This command is pretty similar to "process status" so I think it
makes sense for it to be next to it (in fact, it is so similar, that I'd
consider putting this under some "verbose" switch of process status).
================
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:
> 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.
I don't think it makes sense to include the system definition if you're going
to provide the alternative -- either you support both cross and native
scenarios (in which case you can use the hand-rolled version everywhere), or
you just support the native one (and you just use the system version). That
said, if the struct contains pointer members, I am not sure if you can actually
use the native struct like that. What if the process you're debugging is 32-bit
?
And whatever you do, please don't include a system header in the middle of a
class.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74657/new/
https://reviews.llvm.org/D74657
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
