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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits