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

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added a comment.

This looks great. I just have a quick question about the GPR vs GPRegSet thingy...



================
Comment at: lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp:167
     assert(target_arch.GetTriple().getOS() == llvm::Triple::FreeBSD);
     switch (target_arch.GetMachine()) {
     case llvm::Triple::aarch64:
----------------
omjavaid wrote:
> labath wrote:
> > It would be good to merge these two switches. Then the reg(set)_interface variables would be local to each case label and it would not be so weird that we sometimes use one and sometimes the other.
> I have tried a couple of options but the no of different combinations involved I feel current implementation should stay untill we incrementally move remaining architectures to user RegisterInfosAndSet interface.
I like this solution.


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


================
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
+};
----------------
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...


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:19
 public:
+  enum { ARM64GPR = 0, ARM64FPR };
+
----------------
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.


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

https://reviews.llvm.org/D80105