[Lldb-commits] [PATCH] D127752: [trace][intelpt] Support system-wide tracing [18] - some more improvements

[walter erquinigo via Phabricator via lldb-commits]

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