labath added a comment.

I have two high-level remarks/questions about this:

- I am surprised that it was not necessary to create a special process plugin 
for this purpose. I have a feeling one will be necessary sooner or later 
because of the need to customize the step/continue/etc. flows. Currently, this 
will probably produce very bizarre if one tries to execute those commands. The 
reason I'm bringing this up is because of the `Target::GetTrace` method being 
added in the next patch. If there is a trace-specific process class, then it 
might make sense for this class to hold the trace object instead of adding the 
GetTrace method on every Target object out there even though most targets will 
have that null. I don't know if that will really be the case, but I think it's 
something worth keeping in mind as you work on the subsequent patches.
- I am puzzled by the TraceXXX vs. TraceXXXSettingsParser duality. The two 
classes are very tightly coupled, so it's not clear to me what is the advantage 
of separating it out this way (one could just move all the relevant methods 
directly into the Trace class. What's the reason for this design?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to