clayborg added inline comments.
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:81
+ // and not to a child.
+ lldb::user_id_t GetLastInstructionId() { return m_last_insn_id; }
+
----------------
This seems like it should be stored as an optional value and if the optional
value has no valid, it would need to be computed lazily?
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:83
+
+ size_t GetChildrenCount() { return m_children; }
+
----------------
Should this function be able to lazily compute itself? Right now you would need
to parse the entire stream of instructions for a HTRInMemoryBlock contents to
be valid? Is that what we want?
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:88
+ // most 2^32 elements, which seems reasonable.
+ size_t &GetChildAtIndex(size_t index) { return m_children_indices[index]; }
+
----------------
If you go with the GetChildrenCount() + GetChildAtIndex(...) workflow, then you
can't lazily compute children. Do we want to go with an iterator kind of thing
where you ask for the iterator for the first child and then ask for next until
some ending?
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:90
+
+ llvm::Optional<size_t> GetParent() { return m_parent_index; }
+
----------------
I would avoid size_t as it is uint32_t on 32 bit systems and uint64_t on 64 bit
systems. Probably just pick uint64_t?
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:113
+ // We need this accessor because each block doesn't know its own index
+ HTRInMemoryBlock &GetBlockAtIndex(size_t index) { return m_blocks[index]; }
+
----------------
Bounds check "index"?
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:187-188
+ union {
+ TraceHTRInMemoryStorage *m_in_memory_storage;
+ TraceHTROnDiskStore *m_on_disk_storage;
+ };
----------------
Should we just make a TraceHTR base class that has all of the needed methods
and store a single pointer here? Then TraceHTRInMemoryStorage and
TraceHTROnDiskStore would subclass it?
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:191-192
+ union {
+ HTRInMemoryBlock *m_in_memory_block;
+ HTRInMemoryBlock *m_on_disk_block;
+ };
----------------
Should we just make a HTRBlock base class that has all of the needed methods
and store a single pointer here? Then HTRInMemoryBlock and HTROnDiskBlock would
subclass it?
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:192
+ HTRInMemoryBlock *m_in_memory_block;
+ HTRInMemoryBlock *m_on_disk_block;
+ };
----------------
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:308
+private:
+ std::map<uint32_t, std::vector<size_t>> m_mapping; // symbol id -> block
index
+};
----------------
is the idea to somehow represent a lldb_private::Symbol from this index? What
does a "symbol id" mean?
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:352-355
+ // Only of them active at a given time. An abstract class might also come
+ // handy.
+ std::unique_ptr<TraceHTRInMemoryStorage> m_in_memory_storage;
+ std::unique_ptr<TraceHTROnDiskStorage> m_in_disk_storage;
----------------
Yeah, I would make a base class here and just store one unique pointer here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122859/new/
https://reviews.llvm.org/D122859
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
