[Lldb-commits] [PATCH] D25832: Minidump plugin: Adding x86_32 register context converter

[Pavel Labath via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

looks good, minor comments below.



================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_32.h:37
+// This way we can reuse the already existing register contexts
+lldb::DataBufferSP ConvertMinidumpContext_x86_32_WithRegIface(
+    llvm::ArrayRef<uint8_t> source_data,
----------------
You don't need the `WithRegIface` suffix. That's an ObjC style, which we don't use in LLVM


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_32.h:52
+  enum {
+    RegisteAreaSize = 80,
+  };
----------------
typo


================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_32.h:115
+
+// For context_flags. These values indicate the type of
+// context stored in the structure. The high 24 bits identify the CPU, the
----------------
This comment doesn't seem to apply to the macro below it.


================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:279
 // Register stuff
 // TODO probably split register stuff tests into different file?
+#define REG_VAL32(x) *(reinterpret_cast<uint32_t *>(x))
----------------
If you want to split them off to a different file, do it now. If not, remove the todo. (I don't see a reason for the split btw)


https://reviews.llvm.org/D25832