labath added a comment.
In D89408#2333502 <https://reviews.llvm.org/D89408#2333502>, @wallace wrote:
> Addressed the issues that @labath pointed out regarding the TraceProcess and
> TraceThread:
>
> - These classes were not together
> - TraceProcess was an optional plug-in, which would break the Trace parsing
> logic if plugged-out
>
> So I ended up moving TraceProcess and TraceThread next to Trace in LLDB core,
> guaranteeing that the "trace" process plug-in will always exist and now the
> source file structure looks better.
Yes, that looks better. My only quibble is about the naming. The prevailing
lldb convention is to add a suffix to the subclass name, not prefix. So I think
we should standardize on ProcessTrace & ThreadTrace and not the other way
around.
> Btw, it doesn't seem to me a good idea to not have TraceProcess as a plug-in
> even inside LLDB core, as the main API for constructing processes in Target
> assumes that all processes are plug-ins. I'd rather keep one single simple
> API instead of having two or more.
Yeah, I don't think this is ideal, but it's not that bad either...
================
Comment at: lldb/include/lldb/Target/TraceSessionFileParser.h:18
+typedef std::shared_ptr<TraceThread> TraceThreadSP;
+
----------------
This would be better next to the definition of TraceThread class.
================
Comment at: lldb/include/lldb/Target/TraceSessionFileParser.h:87-101
+ /// Parse the fields common to all trace session schemas.
+ ///
+ /// \param[in] session
+ /// The session json objects already deserialized.
+ ///
+ /// \return
+ //// A list of \a ParsedProcess containing all threads and targets created
----------------
I don't understand the value of this function. Why couldn't the caller just
call `ParseProcesses(session.processes)`?
Are you expecting this to grow and access other subfields of the `session`
object? In that case, maybe it would be better for all `JSONTraceSession<T>`s
to extend a common (non-templated) base class (JSONTraceSettingsBase?) and then
this function could take a reference to that?
================
Comment at: lldb/source/API/SystemInitializerFull.cpp:74
// Now shutdown the common parts, in reverse order.
SystemInitializerCommon::Terminate();
----------------
No terminate?
================
Comment at: lldb/source/Target/TraceProcess.cpp:40
+bool TraceProcess::CanDebug(TargetSP target_sp, bool plugin_specified_by_name)
{
return plugin_specified_by_name;
}
----------------
Maybe this should be just `false`? I don't think we'd ever want to create this
object in response to a normal "launch" request...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89408/new/
https://reviews.llvm.org/D89408
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
