[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace dump ctf -f <filename>` command

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:31
+public:
+  HTRBlockMetadata(Thread &thread, TraceInstruction curr_instruction,
+                   llvm::Optional<TraceInstruction> next_instruction);
----------------
  const TraceInstruction &curr_instruction
to avoid making an unnecessary copy


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:33
+                   llvm::Optional<TraceInstruction> next_instruction);
+  HTRBlockMetadata(size_t num_instructions,
+                   std::unordered_map<std::string, size_t> func_calls)
----------------
pass the map by reference


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:42-68
+  friend llvm::json::Value toJSON(const HTRBlockMetadata &metadata);
+  template <class M> friend llvm::json::Value toJSON(const TraceHTR<M> &layer);
+};
+
+/// \class HTRBlock TraceHTR.h "lldb/Target/TraceHTR.h"
+/// Trace agnostic block structure
+/// Sequences of blocks are merged to create a new, single block
----------------
the consistent style is that toJSON methods are not class members but static functions


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:73
+/// Maps the unique Block IDs to their respective Blocks
+template <class TMetadata> class HTRBlockDefs {
+public:
----------------
clayborg wrote:
> Does this need to be templatized? Are we using this same class with multiple different TMetadata classes or are we always using it with HTRBlockMetadata?
+1
I don't think you should go with templates unless you had more TMetadata objects


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105741