[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked 3 inline comments as done.
omjavaid added inline comments.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp:46
+    : lldb_private::RegisterContext(thread, 0) {
+  m_register_info_up = std::move(register_info);
 
----------------
labath wrote:
> move this to the initializer list
ACK.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:85
+    gpr_w22, gpr_w23,  gpr_w24, gpr_w25, gpr_w26, gpr_w27, gpr_w28,
+    LLDB_INVALID_REGNUM // register sets need to end with this flag
+};
----------------
labath wrote:
> I'd probably just delete this comment (or merge it with the leading comment above the array definition), and then let clang-format lay this out normally...
Let me check what clang-format emits.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:19
 public:
+  enum { ARM64GPR = 0, ARM64FPR };
+
----------------
labath wrote:
> Why these names? I think [GF]PRegSet would be better for two reasons:
> - the same names with the same purpose already exist in `NativeRegisterContextNetBSD_x86_64.h`
> - it seems like a better way to differentiate from the [GF]PR structs below than adding a redundant ARM64 prefix.
Indeed ARM64 is redundant now that we have these enums in RegisterInfosPOSIX_arm64 . I will fix this in updated revision.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80105/new/

https://reviews.llvm.org/D80105