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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Sat Dec 15 05:02:35 PST 2018

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);



More information about the lldb-commits mailing list