amccarth added inline comments.
================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp:1
@@ +1,2 @@
+//===-- Registers_x86_64.cpp ------------------------------------*- C++
-*-===//
+//
----------------
Should match file name.
================
Comment at:
source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp:31
@@ +30,3 @@
+// specified in the RegisterInfoInterface argument
+// This way we can reuse the already existing register contexts
+lldb::DataBufferSP lldb_private::minidump::ConvertMinidumpContextToRegIface(
----------------
It might be better to put this comment with the declaration in the header file,
since it explains what to pass in and what it does. Comments in .cpp files
should contain implementation details that the callers don't necessarily need
to know.
================
Comment at:
source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp:47-48
@@ +46,4 @@
+
+ if (*context_flags & uint32_t(MinidumpContext_x86_64_Flags::Control)) {
+ writeRegister(source_data, result_base, ®_info[lldb_cs_x86_64], 2);
+ }
----------------
dvlahovski wrote:
> If it is then when I do a `&` the result is an enum class of type
> `MinidumpContext_x86_64_Flags`. And the compiler complains that this is not
> convertible to bool
I think what Zach means is that you could locally define a uint32_t const,
initialized with the value from the enum. Then each if statement could use
that constant without a cast.
Also, is this right? `MinidumpContext_x86_x64_Flags::Control` has two bits
set, so the condition will be true if either of them is set. Is that the
intended behavior? Or should you be ensuring that they're both set like this:
const utin32_t ControlFlags = MinidumpContext_x86_64::Control;
if ((*context_flags & ControlFlags) == ControlFlags) {
...
}
?
================
Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h:1
@@ +1,2 @@
+//===-- Registers_x86_64.h --------------------------------------*- C++
-*-===//
+//
----------------
Please make this line match the file name.
================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:177
@@ +176,3 @@
+static void registerEqualToVal(const uint64_t val, uint8_t *reg_val) {
+ ASSERT_EQ(val, *(reinterpret_cast<uint64_t *>(reg_val)));
+}
----------------
+1 to Zach's idea.
Also, consider whether `EXPECT_EQ` is more appropriate than `ASSERT_EQ` for
these.
https://reviews.llvm.org/D24919
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
