[Lldb-commits] [PATCH] D55727: Add "dump" command as a custom "process plugin" subcommand when ProcessMinidump is used.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

+1 for tests. Other than that, the code seems fairly straight-forward, though it could be brought closer to llvm style (e.g., by using more StringRefs in favour of raw c strings).



================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:709-714
+void MinidumpParser::ForEachStreamType(Callback const &callback) const {
+  for (auto const &pair: m_directory_map) {
+    if (!callback(pair.first, pair.second))
+      break;
+  }
+}
----------------
Why not just have an accessor which returns (a const reference to) the list of streams. Then the users can iterate over this any way they see fit (and range-based loops are easier to read than lambda callbacks).


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:93
 
+  static const char *GetStreamTypeAsString(uint32_t stream_type);
+
----------------
Return llvm::StringRef here.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:96-99
+                             const MinidumpLocationDescriptor &loc_desc)>
+      Callback;
+
+  void ForEachStreamType(Callback const &callback) const;
----------------
inconsistent spelling of const references (`const T &` vs `T const &`). I think the first version is more prevalent in both lldb and llvm.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:561
+    ProcessMinidump *process =
+    (ProcessMinidump *)m_interpreter.GetExecutionContext().GetProcessPtr();
+    result.SetStatus(eReturnStatusSuccessFinishResult);
----------------
static_cast


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

https://reviews.llvm.org/D55727