wallace requested review of this revision.
wallace added a comment.
Fixed in https://reviews.llvm.org/D127881
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:205-217
if (data_head > data_size) {
uint64_t actual_data_head = data_head % data_size;
// The buffer has wrapped
- for (uint64_t i = actual_data_head + offset;
- i < data_size && output.size() < size; i++)
+ for (uint64_t i = actual_data_head; i < data_size; i++)
output.push_back(data[i]);
----------------
jj10306 wrote:
> can't this now be simplified to something like:
> ```
> data_head %= data_size;
> // Copy from the head to the end of the buffer.
> output.insert(output.end(), data_head, data_size);
> // Copy from the start of the buffer to the head.
> output.insert(data.end(), data_start, data_head);
> ```
> you will need to make the args to insert ptrs but the rough idea should be
> clear that you can unconditionally modulo the head no need to manually
> iterate any longer, just let insert handle that for you. you might need a
> check to see if the buffer is empty as well.
yes! They will be fixed in https://reviews.llvm.org/D127881
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:257-261
+ for (uint64_t i = aux_head; i < aux_size; i++)
output.push_back(data[i]);
- // We need to find the starting position for the left part if the offset was
- // too big
- uint64_t left_part_start =
- aux_head + offset >= aux_size ? aux_head + offset - aux_size : 0;
- for (uint64_t i = left_part_start; i < aux_head && output.size() < size; i++)
+ for (uint64_t i = 0; i < aux_head; i++)
output.push_back(data[i]);
----------------
jj10306 wrote:
> same comment as above
+1
================
Comment at: lldb/source/Target/Trace.cpp:45
+/// Helper functions for fetching data in maps and returning Optionals or
+/// pointers instead of iterators for simplicity. It's worth mentioning that
the
----------------
jj10306 wrote:
> nit: maybe put these in a separate namespace, up to you though.
they are static, so it's equivalent to having them in an anonymous namespace.
LLVM prefers this approach over creating a specific namespace
================
Comment at: lldb/source/Target/Trace.cpp:67-75
+static Optional<V> Lookup2(DenseMap<K1, DenseMap<K2, V>> &map, K1 k1, K2 k2) {
+ auto it = map.find(k1);
+ if (it == map.end())
+ return None;
+ return Lookup(it->second, k2);
+}
+
----------------
jj10306 wrote:
> nit: potentially better names on these?
I'm just renaming them to Lookup. The 2 is confusing
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127752/new/
https://reviews.llvm.org/D127752
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
