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

[Adrian McCarthy via lldb-commits]

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, &reg_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