[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

I think we should remove the "Stream *s;" from the TraceDumpOptions struct since we have a "Stream &s" as an argument already. We can't use a reference in the TraceDumpOptions without jumping through a lot of hoops.



================
Comment at: lldb/include/lldb/Target/Target.h:1127
+  ///   The trace dump options. See \a TraceDumpOptions for more information.
+  void DumpTrace(Stream &s, lldb_private::TraceDumpOptions &options);
+
----------------
We don't need the stream option as an argument if the TraceDumpOptions has a stream.


================
Comment at: lldb/include/lldb/Target/Trace.h:23
+struct TraceDumpOptions {
+  Stream *s;
+  Process *process;           // If NULL, dump all processes
----------------
We don't need the stream if the dump method takes this as an argment.


================
Comment at: lldb/include/lldb/Target/Trace.h:23
+struct TraceDumpOptions {
+  Stream *s;
+  Process *process;           // If NULL, dump all processes
----------------
clayborg wrote:
> We don't need the stream if the dump method takes this as an argment.
inline init if we leave this in here:

```
Stream *s = nullptr;
```


================
Comment at: lldb/include/lldb/Target/Trace.h:24
+  Stream *s;
+  Process *process;           // If NULL, dump all processes
+  std::set<lldb::tid_t> tids; // Thread IDs, if empty dump all threads
----------------
inline init:

```
Process *process = nullptr;
```


================
Comment at: lldb/include/lldb/Target/Trace.h:26
+  std::set<lldb::tid_t> tids; // Thread IDs, if empty dump all threads
+  bool verbose;
+};
----------------
inline init:
```
bool verbose = false;
```



================
Comment at: lldb/include/lldb/Target/Trace.h:59
+  ///   The trace dump options. See \a TraceDumpOptions for more information.
+  virtual void Dump(lldb_private::Stream &s,
+                    const TraceDumpOptions &options) const = 0;
----------------
Might be nice to leave Stream as an option here since we can use a reference and remove the "Stream *" from the TraceDumpOptions struct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86670