[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:192
+
+static size_t k_num_reg_infos = llvm::array_lengthof(g_reg_infos);
+
----------------
lemo wrote:
> constexpr?
will do


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:195
+// ARM general purpose registers.
+const uint32_t g_gpr_regnums[] = {
+  reg_r0,  reg_r1,  reg_r2,  reg_r3,  reg_r4, reg_r5, reg_r6, reg_r7,
----------------
lemo wrote:
> use std::array for these kind of static arrays? (debug bounds checks, easy access to the static size, ...)
Tried it but it introduces a global constructor. We try to avoid those.


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:56
+  k_num_regs
+};
+
----------------
I would rather define a new context and avoid mutating one register context into another. I didn't really like the other register contexts for minidumps. I like to show the actual data that is available, not a translation of one set of data to another.


https://reviews.llvm.org/D49750