[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);
----------------
static_cast
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55727/new/
https://reviews.llvm.org/D55727
More information about the lldb-commits
mailing list