<div dir="ltr">> - 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.<div><br></div><div>Very good remark. Probably we'll end up doing that when we start implementing reverse debugging. The tricky thing we'll need to solve in the future is seamlessly transition between a trace and a live process. For example, when reverse debugging a live process, you might be stopped at a breakpoint in the trace, then you do "continue", it reaches the end of the trace, and then it resumes the live process. I still haven't decided what we'll propose for this, but probably we'll have to change a lot of the current code to make that happen nicely.</div><div><br></div><div>> - 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?<br></div><div><br></div><div>Well, there's definitely coupling, but there are two reasons. One is that the Trace of a live process won't need any parsing. Parsing is only used when loading a trace from a json file. That makes parsing one of the two main ways to create a Trace. And besides, I think that the single responsibility principle is good to follow. The Parser class does the parsing, and the Trace class holds an actual trace.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Il giorno gio 1 ott 2020 alle ore 07:12 Pavel Labath via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> ha scritto:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">labath added a comment.<br>
<br>
I have two high-level remarks/questions about this:<br>
<br>
- 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.<br>
- 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?<br>
<br>
<br>
Repository:<br>
  rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D85705/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D85705/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D85705" rel="noreferrer" target="_blank">https://reviews.llvm.org/D85705</a><br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>- Walter Erquínigo Pezo<br><br></div></div></div>