[Lldb-commits] [PATCH] D24919: Adding a RegisterContextMinidump_x86_64 converter

[Pavel Labath via lldb-commits]

labath added inline comments.

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:42
@@ -41,1 +41,3 @@
 
+  const uint8_t *GetBaseAddr();
+
----------------
Replace these two functions with `llvm::ArrayRef<uint8_t> GetData() const`

================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h:55
@@ +54,3 @@
+
+struct MinidumpContext_x86_64 {
+  // Register parameter home addresses.
----------------
This function does not reflect the actual memory layout of the data, and you are not using it. I think it should go away.

================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h:161
@@ +160,3 @@
+
+  static void writeRegister(llvm::ArrayRef<uint8_t> &data, uint8_t *base,
+                            const RegisterInfo *reg_info, size_t size);
----------------
This looks like a private utility function. It should not be in the header.

================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h:186
@@ +185,3 @@
+};
+}
+}
----------------
Could you add ` // end namespace minidump` comments. Without them it's hard to tell what is going on here.

================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:176
@@ +175,3 @@
+// TODO probably split register stuff tests into different file?
+void registerEqualToVal(const uint64_t val, uint8_t *reg_val) {
+  ASSERT_EQ(val, *(reinterpret_cast<uint64_t *>(reg_val)));
----------------
`static`

Also i think having the register tests in this file is fine.


https://reviews.llvm.org/D24919